llvm dev - Sep 2009 - [LLVMdev] [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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spillcomments.patch
Type: text/x-diff
Size: 58930 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090911/9c86795e/attachment.patch>

On Friday 11 September 2009 17:31, you wrote:
> 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.
No feedback?  What's one supposed to do in such cases?

                           -Dave

On Sep 14, 2009, at 10:06 AM, David Greene wrote:
> On Friday 11 September 2009 17:31, you wrote:
>> 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.
>
> No feedback?  What's one supposed to do in such cases?
Generally you just ping it, as you did.  I'll take a look now.

-Chris

Hi Dave,

On Sep 11, 2009, at 3:31 PM, David Greene wrote:
> 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

On Monday 14 September 2009 12:22, Dan Gohman wrote:
> Hi Dave,
>
> On Sep 11, 2009, at 3:31 PM, David Greene wrote:
> > 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

On Sep 11, 2009, at 3:31 PM, David Greene wrote:
> 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!


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

On Sep 14, 2009, at 10:22 AM, Dan Gohman wrote:
> Hi Dave,
>
> On Sep 11, 2009, at 3:31 PM, David Greene wrote:
>
>> 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.
It should just be a vector which is binary searched.  I think that  
will make it cheap enough even with asmverbose off.

-Chris

On Monday 14 September 2009 12:32, Chris Lattner wrote:
> On Sep 11, 2009, at 3:31 PM, David Greene wrote:
> > 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.  :)
> 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