[PATCH] Spill Comments

Attached is a patch to print asm comments for spill information.
We've discussed the mechanisms before but I wanted to run the
patch by everyone before I start to commit pieces.

                             -Dave

spillcomments.patch (57.5 KB)

No feedback? What's one supposed to do in such cases?

                           -Dave

Generally you just ping it, as you did. I'll take a look now.

-Chris

Hi Dave,

Attached is a patch to print asm comments for spill information.
We've discussed the mechanisms before but I wanted to run the
patch by everyone before I start to commit pieces.

The Offset->FrameIndex mapping seems rather heavy-weight, as
any expense is incurred even when AsmVerbose is off. Would it
be possible to use MachineMemOperands instead? In theory,
they should already be preserving the needed information. If
they're not sufficient, could they be improved?

There is work going on to improve the register allocator's ability
to use rematerlialization. In a world where the register allocator
can aggressively remat misc. loads, will it still be interesting to
identify spill reloads?

You mentioned at one point that you have some scripts which
know how to read some of these comments. Would it be
possible for you to include these scripts with the patch so that
others can consider your patch in context?

Dan

Hi Dave,

> Attached is a patch to print asm comments for spill information.
> We've discussed the mechanisms before but I wanted to run the
> patch by everyone before I start to commit pieces.

The Offset->FrameIndex mapping seems rather heavy-weight, as
any expense is incurred even when AsmVerbose is off. Would it
be possible to use MachineMemOperands instead? In theory,
they should already be preserving the needed information. If
they're not sufficient, could they be improved?

Yeah, I'm not totally happy with that mapping either. With
MachineMemOperands, would that be the getOffset() method? That's
only for FPRel data, though. What if frame pointer elimination has
been run?

I would love to get out from under the map if possible.

There is work going on to improve the register allocator's ability
to use rematerlialization. In a world where the register allocator
can aggressively remat misc. loads, will it still be interesting to
identify spill reloads?

Yes. I have a later patch that marks remats as well. The idea is to get a
sense of how well regalloc is doing. And you can't remat everything.

You mentioned at one point that you have some scripts which
know how to read some of these comments. Would it be
possible for you to include these scripts with the patch so that
others can consider your patch in context?

I'm not sure. The script counts a whole bunch of things. All it does is
look for these commentsw and tally up statistics, what's in loops, etc.
It's pretty straightforward.

                                   -Dave

Some thoughts:

The general approach to enhancing CreateStackObject and adding MachineInstr::AsmPrinterFlags seems fine to me!

The testcase should use filecheck to avoid running llc 4 times. Also, it seems better to design a situation where you just have 16 live variables instead of taking some random function for gcc (you're implicitly depending on how ra is handling this code instead of forcing a situation where any x86-64 codegen would have to spill).

The constness change to TargetRegisterInfo::getFrameRegister looks great, please commit it separately.

+ /// hasLoadFromStackSlot - If the specified machine instruction is a
+ /// direct load from a stack slot, return true along with the
+ /// FrameIndex of the loaded stack slot. If not, return false.
+ /// Unlike isLoadFromStackSlot, this returns true for any
+ /// instructions that loads from the stack.

This comment is contradictory. It returns true if it is a "direct load" but "returns true for any instructions that loads". likewise with the comment on hasStoreToStackSlot. Would it make sense for the default impl of hasLoadFromStackSlot to be isLoadFromStackSlot?

It might be worthwhile to say that this is a "best effort" method. It is not guaranteed to return true for all instructions of the form, it is just a hint.

A couple of the methods you're adding to MachineFrameInfo.h are large, they should be moved to the .cpp file, allowing you to avoid <limits> in the header.

+ /// SpillObjects - A set of frame indices represnting spill slots.

typo represnting.

Why does SpillObjects need to be a DenseSet? It seems that it is created in order. I think it can just be a vector which is looked up with a binary search.

Instead of making CreateStackObject take a "bool isSpill", how about adding a new "CreateSpillStackObject"? That seems much nicer in the code than having: CreateStackObject(16, 16, /*isSpill*/false); everywhere. The number of places that create spill slots is pretty small compared to the number of "/*isSpill*/false".

What is the lib/Target/X86/X86RegisterInfo.cpp change doing? It needs a comment.

+bool X86InstrInfo::isFrameOperand(const MachineInstr *MI, unsigned int Op,
..
+ return true;
+ } else {
+ // Check for post-frame index elimination

Please eliminate the 'else' to avoid nesting. Also, please terminate your comment with a period. There are several comments in the patch that need to be terminated.

In AsmPrinter::EmitComments please make "Newline" be a bool "NeedsNewline" instead of a char with only two states (uninitialized and '\n'). Please initialize it.

Overall, the approach looks very nice. Please commit this (with the changes above) as a series of patches:

1. Constify the TargetRegisterInfo::getFrameRegister argument.
2. Change CreateStackObject so MFI can maintain the information it needs.
3. Add the MachineInstr AsmPrinter flags stuff.
4. Land the Asmprinter changes themselves.

Thanks David,

-Chris

It should just be a vector which is binary searched. I think that will make it cheap enough even with asmverbose off.

-Chris

Are MachineMemOperands available at AsmPrinter time? Where are they stashed?

                               -Dave

> Attached is a patch to print asm comments for spill information.
> We've discussed the mechanisms before but I wanted to run the
> patch by everyone before I start to commit pieces.

Some thoughts:

The general approach to enhancing CreateStackObject and adding
MachineInstr::AsmPrinterFlags seems fine to me!

Ok.

The testcase should use filecheck to avoid running llc 4 times. Also,
it seems better to design a situation where you just have 16 live
variables instead of taking some random function for gcc (you're
implicitly depending on how ra is handling this code instead of
forcing a situation where any x86-64 codegen would have to spill).

I just took some existing spill tests, but you're point is fair.

The constness change to TargetRegisterInfo::getFrameRegister looks
great, please commit it separately.

Will do.

+ /// hasLoadFromStackSlot - If the specified machine instruction is a
+ /// direct load from a stack slot, return true along with the
+ /// FrameIndex of the loaded stack slot. If not, return false.
+ /// Unlike isLoadFromStackSlot, this returns true for any
+ /// instructions that loads from the stack.

This comment is contradictory. It returns true if it is a "direct
load" but "returns true for any instructions that loads". likewise

Cut & paste error. :slight_smile:

with the comment on hasStoreToStackSlot. Would it make sense for the
default impl of hasLoadFromStackSlot to be isLoadFromStackSlot?

Yeah, goo idea.

It might be worthwhile to say that this is a "best effort" method. It
is not guaranteed to return true for all instructions of the form, it
is just a hint.

Right.

A couple of the methods you're adding to MachineFrameInfo.h are large,
they should be moved to the .cpp file, allowing you to avoid <limits>
in the header.

Ok.

Why does SpillObjects need to be a DenseSet? It seems that it is
created in order. I think it can just be a vector which is looked up
with a binary search.

I wasn't sure ordering was guaranteed. As I noted to Dan, I'd like to get
rid of this entirely.

Instead of making CreateStackObject take a "bool isSpill", how about
adding a new "CreateSpillStackObject"? That seems much nicer in the
code than having: CreateStackObject(16, 16, /*isSpill*/false);
everywhere. The number of places that create spill slots is pretty
small compared to the number of "/*isSpill*/false".

Yep, much better.

What is the lib/Target/X86/X86RegisterInfo.cpp change doing? It needs
a comment.

Ok. It's just setting the mapping of stack offset to FrameIndex.

                            -Dave

Yes. There are some places where they aren't preserved, but otherwise
they stick around for the entire CodeGen process. The places that
don't preserve them should ideally be fixed eventually.

They're available via the memoperands_begin()/memoperands_end()
MachineInstr member functions.

Dan

I'm pretty sure it is: the "is spill slot" is a property set when the spill is created, and spill slots are assigned increasing numbers in order.

-Chris

MachineMemOperands for spill slots use FixedStack PseudoSourceValues
as their base. There's a unique FixedStack PseudoSourceValue for each
fixed frame object, so it's independent of whether frame pointer
elimination has been done, and it's independent of the actual frame
offsets.

Dan

Ok. Since I'm using a variant of isLoadFromStackSlot to figure things out,
should we consider rewriting that function to look at memoperands rather
than operands?

Eventually, I mean, not part of this patch.

                           -Dave

/// getValue - Return the base address of the memory access.
  /// Special values are PseudoSourceValue::FPRel, PseudoSourceValue::SPRel,
  /// and the other PseudoSourceValue members which indicate references to
  /// frame/stack pointer relative references and other special references.
  const Value *getValue() const { return V; }

I don't see PseudoSourceValue::FPRel, etc. defined anywhere. How do I know
if a PseudoSourceValue is from the stack?

                              -Dave

Ok, the comment is misleading. I see the class defined in
PseudoSourceValue.cpp now. I'll move it to the header.

I have another question. Looking at the list of MachineMemOperands for an
instruction, is ther eany way to easily know whether the operand is loaded
from or stored to?

                            -Dave

I don't see PseudoSourceValue::FPRel, etc. defined anywhere. How do I know

if a PseudoSourceValue is from the stack?

Ok, the comment is misleading. I see the class defined in
PseudoSourceValue.cpp now. I'll move it to the header.

Yes; the comment was out of date. I just updated it.

I have another question. Looking at the list of MachineMemOperands for an
instruction, is ther eany way to easily know whether the operand is loaded
from or stored to?

Yes. MachineMemOperand has isLoad and isStore flags.

Dan