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 Jul 13, 2009, at 9:31 AM, David Greene 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. > > I was attempting to reduce the number of files affected, but if you > want this change I'll go ahead and do it.Makes sense, thanks. Please do it as a separate patch from the other changes though since it will be large and mechanical.> >> 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).Right, that would be nice. This should be synthesizable from the DebugLoc on the instruction in the asm printer, no need to redundantly encode it into the comment field.> - Tag instructions as register spills or reloads.I'm not sure what this means exactly, but would it be sufficient for the asmprinter to use isLoadFromStackSlot and print this if the FI is a spill slot?> - Tag instructions with an ID of the tblgen pattern that generated > them. This > is super useful for debugging.this would also be really nice :). This can be generated by the asm printer as well.> - Tag instructions as top-of-loop, with nesting information (we use > this > to do some static analysis on asm files).What part of the code generator would identify this? It seems that the asmprinter could do this, but it is less obvious than the former ones. It also seems that this is really a basic block property, not an instruction property. If these are the planned uses of the comments, it would be nice try to not add a per-machine-instr list of comments. Instead, the asmprinter could synthesize the list as it processes each instruction. This makes the list of comments transient instead of persistent in the machineinstrs. Does that sound reasonable to you? -Chris
On Monday 13 July 2009 11:40, Chris Lattner wrote:> > I was attempting to reduce the number of files affected, but if you > > want this change I'll go ahead and do it. > > Makes sense, thanks. Please do it as a separate patch from the other > changes though since it will be large and mechanical.Ok, no problem.> > - Tag instructons with source line information (customers really > > want this). > > Right, that would be nice. This should be synthesizable from the > DebugLoc on the instruction in the asm printer, no need to redundantly > encode it into the comment field.Except the DebugLoc stuff isn't all there yet AFAIK. We've been using the comment mechanism for over a year. I agree we should move to synthesizing it from DebugLoc when it's ready. We're not going to submit our line number stuff anyway (it's too much of a hack) but we would like the comment infrastructure to be there.> > - Tag instructions as register spills or reloads. > > I'm not sure what this means exactly, but would it be sufficient for > the asmprinter to use isLoadFromStackSlot and print this if the FI is > a spill slot?Maybe. I'm not sure what information is available here. The other thing this code does is tag it as a vector or scalar spill/reload. Synthesizing that might be trickier as you have to take the opcode into account. It's a lot of work, at the very least.> > - Tag instructions with an ID of the tblgen pattern that generated > > them. This > > is super useful for debugging. > > this would also be really nice :). This can be generated by the asm > printer as well.How so? Where is the pattern information available?> > - Tag instructions as top-of-loop, with nesting information (we use > > this > > to do some static analysis on asm files). > > What part of the code generator would identify this? It seems that > the asmprinter could do this, but it is less obvious than the former > ones. It also seems that this is really a basic block property, not > an instruction property.Yes, we tag basic blocks. That patch is coming later. I added a pass that explicitly examines loop information and adds the appropriate comments. So this could be synthesized on-the-fly, I think.> If these are the planned uses of the comments, it would be nice try to > not add a per-machine-instr list of comments. Instead, the asmprinter > could synthesize the list as it processes each instruction. This > makes the list of comments transient instead of persistent in the > machineinstrs. Does that sound reasonable to you?I still don't know how to synthesize tblgen pattern information in the asmprinter. Except line numbers, which we need today. Maybe all of the other current stuff can be synthesized. I don't know what might come down the road, though. What's the plan for meta-information? Could comments go there when it's ready? Would it be ok to add these for now and remove them as other mechanisms (DebugLoc, meta-information) come on-line? -Dave
On Monday 13 July 2009 11:40, Chris Lattner wrote:> > - Tag instructions as register spills or reloads. > > I'm not sure what this means exactly, but would it be sufficient for > the asmprinter to use isLoadFromStackSlot and print this if the FI is > a spill slot?How does one know if the FI is a spill slot? -Dave