I'm getting to the point where I want to contribute some more
MachineInstr comment support for things like spills. As we've
discussed before, we don't have all of the information available
in AsmPrinter to synthesize the kind of comments that can be
helpful for debugging performance issues with register allocators
(our primary use for these kinds of comments).
In order to get this information to the AsmPrinter without requring
a lot of overhead, I'd like to propose adding a small bitvector to
MachineInstr to hold flags. Something like this:
class MachineInstr : public ilist_node<MachineInstr> {
const TargetInstrDesc *TID; // Instruction descriptor.
unsigned short NumImplicitOps; // Number of implicit operands (which
// are determined at construction
time).
unsigned short Flags; // ***NEW*** Various bits of info
std::vector<MachineOperand> Operands; // the operands
std::list<MachineMemOperand> MemOperands; // information on memory
references
MachineBasicBlock *Parent; // Pointer to the owning basic block.
DebugLoc debugLoc; // Source line information.
Adding Flags between NumImplicitOps and Operands shouldn't increase the size
of MachineInstr on most platforms due to alignment padding.
The kind of information I'd like to transmit via Flags is:
- This register-register copy is a result of a reload value being reused
(i.e. it was inserted by the spiller).
- This spill was induced by some decision made by the allocator (ex. graph
coloring choosing to speculatively color some node which caused a cascade
effect).
- This spill was forced by some debugging flag (this is coming in a later
set of patches).
We could also mark instructions as spills but Chris has pointed out that we
can probably synthesize that in AsmPrinter with just a little bit of work.
The above information can't be inferred from stack layouts or anything else.
Thoughts?
-Dave
On Jul 31, 2009, at 11:17 AM, David Greene wrote:> I'm getting to the point where I want to contribute some more > MachineInstr comment support for things like spills. As we've > discussed before, we don't have all of the information available > in AsmPrinter to synthesize the kind of comments that can be > helpful for debugging performance issues with register allocators > (our primary use for these kinds of comments). > > In order to get this information to the AsmPrinter without requring > a lot of overhead, I'd like to propose adding a small bitvector to > MachineInstr to hold flags. Something like this: > > class MachineInstr : public ilist_node<MachineInstr> { > const TargetInstrDesc *TID; // Instruction descriptor. > unsigned short NumImplicitOps; // Number of implicit > operands (which > // are determined at > construction > time). > > unsigned short Flags; // ***NEW*** Various bits of > infoThose bits are almost asking to be used ;-).> > std::vector<MachineOperand> Operands; // the operands > std::list<MachineMemOperand> MemOperands; // information on memory > references > MachineBasicBlock *Parent; // Pointer to the owning > basic block. > DebugLoc debugLoc; // Source line information. > > Adding Flags between NumImplicitOps and Operands shouldn't increase > the size > of MachineInstr on most platforms due to alignment padding. > > The kind of information I'd like to transmit via Flags is: > > - This register-register copy is a result of a reload value being > reused > (i.e. it was inserted by the spiller). > > - This spill was induced by some decision made by the allocator (ex. > graph > coloring choosing to speculatively color some node which caused a > cascade > effect). > > - This spill was forced by some debugging flag (this is coming in a > later > set of patches). > > We could also mark instructions as spills but Chris has pointed out > that we > can probably synthesize that in AsmPrinter with just a little bit of > work. > The above information can't be inferred from stack layouts or > anything else. > > Thoughts?I'm ok with this, in abstract. It would be good to put up some intimidating comments to scare people away from using these flags for anything except for the AsmPrinter, because semantics are tricky enough as they are right now without new subtle bits to worry about. It even occurs to me to suggest putting the bits in a private member in a class and making AsmPrinter a friend of that class in order to discourage accidental misuse, but that may be overreacting :-). Dan
On Saturday 01 August 2009 15:20, Dan Gohman wrote:> > unsigned short Flags; // ***NEW*** Various bits of > > info > > Those bits are almost asking to be used ;-).I'll rename them AsmPrinterFlags.> I'm ok with this, in abstract. It would be good to put up some > intimidating > comments to scare people away from using these flags for anything > except for the AsmPrinter, because semantics are tricky enough as they > are right now without new subtle bits to worry about.I agree.> It even occurs to me to suggest putting the bits in a private member in > a class and making AsmPrinter a friend of that class in order to > discourage accidental misuse, but that may be overreacting :-).I really don't like the idea of opening up all of MachineInstr's guts to AsmPrinter. I wish C++ had a more selective concept of friend. -Dave
On Aug 1, 2009, at 1:20 PM, Dan Gohman wrote:>> We could also mark instructions as spills but Chris has pointed out >> that we >> can probably synthesize that in AsmPrinter with just a little bit of >> work. >> The above information can't be inferred from stack layouts or >> anything else. >> >> Thoughts? > > I'm ok with this, in abstract. It would be good to put up some > intimidating > comments to scare people away from using these flags for anything > except for the AsmPrinter, because semantics are tricky enough as they > are right now without new subtle bits to worry about.I agree that this is a good approach, and that this should only be used for "random comments", the bits shouldn't have a semantic effect on the code. Just put a strong comment on them, and that is enough.> It even occurs to me to suggest putting the bits in a private member > in > a class and making AsmPrinter a friend of that class in order to > discourage accidental misuse, but that may be overreacting :-).Yeah, probably not necessary ;) -Chris