Here's the first of several patches to get comments into asm output. This
one
adds comment information to MachineInstructions and outputs it in the
generated AsmPrinters. This includes TableGen work to trigger the comment
output in the right places.
Please review and comment.
-Dave
-------------- next part --------------
A non-text attachment was scrubbed...
Name: comment.patch
Type: text/x-diff
Size: 7907 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090710/3706bf69/attachment.patch>
On Jul 10, 2009, at 3:05 PM, David Greene wrote:> Here's the first of several patches to get comments into asm > output. This one > adds comment information to MachineInstructions and outputs it in the > generated AsmPrinters. This includes TableGen work to trigger the > comment > output in the right places.A couple of things are important to discuss: + dynamic_cast<formatted_raw_ostream &>(O) << Comment(*c); We're trying to eliminate rtti, please don't add new uses of it. Switching all of the asmprinter to statically use formatted_raw_ostream would be appropriate. +++ include/llvm/CodeGen/MachineInstr.h (working copy) @@ -46,6 +46,9 @@ MachineBasicBlock *Parent; // Pointer to the owning basic block. DebugLoc debugLoc; // Source line information. + typedef std::vector<std::string> CommentList; + CommentList Comments; // asm file comments This is a very heavy-weight representation, growing each MachineInstr by 3 words. Once .o file writing comes on-line, the performance of the .s file writer will matter much less, but this growth will still affect it. Before we settle on whether this is the right thing to do or not, can you talk about what comments you plan to add to these instructions? Comments indicating what a memoperand are (for example) don't need to be explicitly store in the machineinstr, they can be synthesized from the MachineOperand directly. Minor stuff: + /// commentsBegin - Get an iterator to the start of the comment list + /// + const_comment_iterator commentsBegin(void) const { + return(Comments.begin()); + } no need for the (void), no need for the redundant parens, please punctuate comments properly. -Chris
On Sunday 12 July 2009 18:56, Chris Lattner wrote:> On Jul 10, 2009, at 3:05 PM, David Greene wrote: > > Here's the first of several patches to get comments into asm > > output. This one > > adds comment information to MachineInstructions and outputs it in the > > generated AsmPrinters. This includes TableGen work to trigger the > > comment > > output in the right places. > > A couple of things are important to discuss: > > + dynamic_cast<formatted_raw_ostream &>(O) << Comment(*c); > > We're trying to eliminate rtti, please don't add new uses of it. > Switching all of the asmprinter to statically use > formatted_raw_ostream would be appropriate.I was attempting to reduce the number of files affected, but if you want this change I'll go ahead and do it.> Before we settle on whether this is the right thing to do or not, can > you talk about what comments you plan to add to these instructions? > Comments indicating what a memoperand are (for example) don't need to > be explicitly store in the machineinstr, they can be synthesized from > the MachineOperand directly.Some things we've used comments for: - Tag instructons with source line information (customers really want this). - Tag instructions as register spills or reloads. - Tag instructions as top-of-loop, with nesting information (we use this to do some static analysis on asm files). - Tag instructions with an ID of the tblgen pattern that generated them. This is super useful for debugging. -Dave
On Sunday 12 July 2009 18:56, Chris Lattner wrote:> A couple of things are important to discuss: > > + dynamic_cast<formatted_raw_ostream &>(O) << Comment(*c); > > We're trying to eliminate rtti, please don't add new uses of it. > Switching all of the asmprinter to statically use > formatted_raw_ostream would be appropriate.To separate comments, would it be ok to commit this bit as-is and follow it up with making the change to static formatted_raw_ostream? -Dave