[LLVMdev] [PATCH] Spill Comments

[David Greene]

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