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". likewiseCut & 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