RFC: MachineInstr Annotations

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

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

Those 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

> 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

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 :wink:

-Chris