[LLVMdev] [PATCH] Spill Comments
dag at cray.com
Mon Sep 14 10:50:54 PDT 2009
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!
> 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.
> + /// 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.
> 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.
> 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.
More information about the llvm-dev