[LLVMdev] [PATCH] Spill Comments

[Chris Lattner]

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