[PATCH] Support asm comment output

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

comment.patch (7.72 KB)

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

> 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

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

> 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

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

Yep, I'm fine with it being that way temporarily, please don't add the comments vector to MachineInstr though.

-Chris

- 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.

DebugLoc is there. The transition isn't complete at the LLVM IR level, but it is at the MachineInstr level AFAIK.

- 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.

Vector vs scalar should also be pretty simple, just look at the reg class and the VT's involved. It should all be target independent, probably less than a dozen lines. After the conversion to formatted_raw_ostream, we can discuss the best way to do this. It would be helpful if you gave an example of the output you wanted.

- 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?

You just want the name of the enum? This is the name field of the TargetInstrDesc. If -print-machineinstrs has it, asmprinter can just as easily :).

- 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.

Ok, my question is: "why in a separate pass instead of in the asmprinter"? The idea is that comments inherently have no semantic value, and they are only useful for the asmprinter backend. To reduce complexity and compile time cost, it makes sense to do this stuff in the asmprinter if feasible.

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.

I don't know what you mean by "pattern information". We can obviously enhance tblgen to include any information that we need, but I don't see how the asmprinter wouldn't know something that an earlier pass would. Again, it would be helpful if you included example output to show what you're going for.

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?

Which meta-information, MDNode and friends? These won't really be per-machine-instruction, so that won't help. There is a possibility that we can add MD operands to machineinstrs, which might be sufficient for your needs, but again it is unclear why you want to do any of this stuff earlier than asmprinter.

-Chris

> 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.

DebugLoc is there. The transition isn't complete at the LLVM IR
level, but it is at the MachineInstr level AFAIK.

Well, the IR level is pretty important to us since we convert our
code into LLVM IR and add source line comments at the same time.

Vector vs scalar should also be pretty simple, just look at the reg
class and the VT's involved. It should all be target independent,
probably less than a dozen lines. After the conversion to
formatted_raw_ostream, we can discuss the best way to do this. It
would be helpful if you gave an example of the output you wanted.

Ok, this might work.

I don't know what you mean by "pattern information". We can obviously
enhance tblgen to include any information that we need, but I don't
see how the asmprinter wouldn't know something that an earlier pass
would. Again, it would be helpful if you included example output to
show what you're going for.

Ok, here's an example:

        movl (%rsi), %ecx # LLVM
Instruction: volatile store i32* %ksize, i32** %ksize3 ;
[oox.4 : sln.5]

                                                            # [result] Pattern
1367

I then hacked tblgen to output this in the generated isel file:

    // Pattern 1367: (ld:i32 addr:iPTR:$src)<<P:Predicate_load>>
    // Emits: (MOV32rm:i32 addr:iPTR:$src)
    PatternID = 1367;
    // Pattern complexity = 19 cost = 1 size = 3
    SDValue CPTmp01;
    SDValue CPTmp11;
    SDValue CPTmp21;
    SDValue CPTmp31;
    if (SelectAddr(N, N1, CPTmp01, CPTmp11, CPTmp21, CPTmp31)) {
      CurDAG->setSubgraphColor(N.getNode(), "red");
      SDNode *Result = Emit_125(N, X86::MOV32rm, MVT::i32, CPTmp01, CPTmp11,
CPTmp21, CPTmp31, PatternID);
      if(Result) {
        CurDAG->setSubgraphColor(Result, "yellow");
        CurDAG->setSubgraphColor(Result, "black");
      }
      return Result;
    }
  }

So it's really easy to set a breakpoint in Emit_125 to look for the pattern
number in question.

I forgot about the LLVM instruction information. That's something that can't
be synthesized by the asmprinter. Again, we only emit some of this with
special debug flags so we don't carry the original IR around in comments all
the time. :slight_smile:

> 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?

Which meta-information, MDNode and friends?

I don't know, I've just heard the term from time to time. Sounds like it's
not appropriate for this, though.

                               -Dave

How does one know if the FI is a spill slot?

                             -Dave

If the FI question can be answered (how do we know it's a spill slot)?
Then the above is the only use I think we don't have a long-term answer for.
I don't see how asmprinter can synthesize this information.

Line numbers are still a problem because we need to propagate the information
from LLVM IR to MachineInstrs and there's currently no DebugLoc information
in LLVM IR Instructions. I'll take a look at the existing DebugLoc and see if
we can put our LLVM IR line number info there instead of printing it to
comments early.

If it's any comfort, we've been running with this MachineInstr for a
very long time and done multiple public releases. We always go through
asm to do our x86 compiles and no one has ever complained about compile
time or memory usage in this area (legalize is actually our biggest problem).
And we compile HUGE source files containing very large functions.

Of course, our requirements aren't the same as everyone else's requirements
so I understand that there may be tighter tolerances in some use cases.
Examples of where the MachineInstr comments might be a problem would be
helpful.

                              -Dave

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.

DebugLoc is there. The transition isn't complete at the LLVM IR
level, but it is at the MachineInstr level AFAIK.

Well, the IR level is pretty important to us since we convert our
code into LLVM IR and add source line comments at the same time.

Well sure, but how is that relevant? Just generate llvm.stoppoint and isel turns them into debug locs. Just because debugloc isn't on llvm::Instruction doesn't mean you don't get line numbers :).

I don't know what you mean by "pattern information". We can obviously
enhance tblgen to include any information that we need, but I don't
see how the asmprinter wouldn't know something that an earlier pass
would. Again, it would be helpful if you included example output to
show what you're going for.

Ok, here's an example:

       movl (%rsi), %ecx # LLVM
Instruction: volatile store i32* %ksize, i32** %ksize3 ;
[oox.4 : sln.5]

                                                           # [result] Pattern
1367

Instead of printing 1367, why not print "MOV32rm" like -print-machineinstr does? If you really want 1367, just print MI- >getOpcode(). The asmprinter definitely has this information.

I forgot about the LLVM instruction information. That's something that can't
be synthesized by the asmprinter. Again, we only emit some of this with
special debug flags so we don't carry the original IR around in comments all
the time. :slight_smile:

Sure, I understand that comments in general only get turned on with something like -fverbose-asm. But where/how do you expect this information to be propagated onto the machineinstrs?

For now, lets go forward with the plan of having the asmprinter generate the comments. It sounds like it can cover at least 75% of what you're interested in, and the other cases probably have better solutions that sticking a vector of std::string on MachineInstr.

-Chris

We don't currently track this in MachineFrameInfo, but it would be easy (and cheap) to add.

-Chris

I forgot about the LLVM instruction information. That's something that
can't be synthesized by the asmprinter. Again, we only emit some of this
with special debug flags so we don't carry the original IR around in
comments all the time. :slight_smile:

If the FI question can be answered (how do we know it's a spill slot)?

I just responded separately.

Then the above is the only use I think we don't have a long-term answer for.
I don't see how asmprinter can synthesize this information.

I agree, but our isel currently doesn't propagate this in any other way either. Lets just defer this issue for now.

Line numbers are still a problem because we need to propagate the information
from LLVM IR to MachineInstrs and there's currently no DebugLoc information
in LLVM IR Instructions. I'll take a look at the existing DebugLoc and see if
we can put our LLVM IR line number info there instead of printing it to
comments early.

Just generate stoppoints like llvm-gcc.

If it's any comfort, we've been running with this MachineInstr for a
very long time and done multiple public releases. We always go through
asm to do our x86 compiles and no one has ever complained about compile
time or memory usage in this area (legalize is actually our biggest problem).
And we compile HUGE source files containing very large functions.

We care most about -O0 compile times, which generally don't use legalize or selection dags at all, and use the local register allocator.

-Chris

Well sure, but how is that relevant? Just generate llvm.stoppoint and
isel turns them into debug locs. Just because debugloc isn't on
llvm::Instruction doesn't mean you don't get line numbers :).

You're right, it's not a big hurdle.

> Ok, here's an example:
>
> movl (%rsi), %ecx # LLVM
> Instruction: volatile store i32* %ksize, i32** %ksize3 ;
> [oox.4 : sln.5]
>
> #
> [result] Pattern
> 1367

Instead of printing 1367, why not print "MOV32rm" like -print-
machineinstr does? If you really want 1367, just print MI-

>getOpcode(). The asmprinter definitely has this information.

Ok, I wasn't aware of that at the time I did this stuff. Printing the
pattern name is even better!

> I forgot about the LLVM instruction information. That's something
> that can't
> be synthesized by the asmprinter. Again, we only emit some of this
> with
> special debug flags so we don't carry the original IR around in
> comments all
> the time. :slight_smile:

Sure, I understand that comments in general only get turned on with
something like -fverbose-asm. But where/how do you expect this
information to be propagated onto the machineinstrs?

Some comments are on all the time, but those I think we can do with
asmprinter, as you suggest.

I have patches that propagate comments from Instructions to SDNodes to
MachineInstrs.

Again, after answering the FI stack slot question, the IR-level instruction
information is the only thing we can't do through asmprinter. I'm open to
other ways of capturing the information that don't require comments in
MachineInstrs. But the information has to be stored somewhere.

This IR instruction stuff isn't a huge priority for us. It's helpful
when debugging but the TableGen information is way more important. I
wouldn't cry too much if we can't get the IR instruction information
in right now.

For now, lets go forward with the plan of having the asmprinter
generate the comments. It sounds like it can cover at least 75% of
what you're interested in, and the other cases probably have better
solutions that sticking a vector of std::string on MachineInstr.

That's ok by me. Let's hash through this some more as stuff comes on-line.

                              -Dave

Ok, I'll look at doing that as part of this work.

                           -Dave

Ok, here's an example:

      movl (%rsi), %ecx # LLVM
Instruction: volatile store i32* %ksize, i32** %ksize3 ;
[oox.4 : sln.5]

                                                          #
[result] Pattern
1367

Instead of printing 1367, why not print "MOV32rm" like -print-
machineinstr does? If you really want 1367, just print MI-

getOpcode(). The asmprinter definitely has this information.

Ok, I wasn't aware of that at the time I did this stuff. Printing the
pattern name is even better!

Oh, also, if it's not clear :), I'm really excited by this work. Making the .s files more readable would be a huge quality of life improvement for many people working on the code generator!

I forgot about the LLVM instruction information. That's something
that can't
be synthesized by the asmprinter. Again, we only emit some of this
with
special debug flags so we don't carry the original IR around in
comments all
the time. :slight_smile:

Sure, I understand that comments in general only get turned on with
something like -fverbose-asm. But where/how do you expect this
information to be propagated onto the machineinstrs?

Some comments are on all the time, but those I think we can do with
asmprinter, as you suggest.

Yep.

I have patches that propagate comments from Instructions to SDNodes to
MachineInstrs.
Again, after answering the FI stack slot question, the IR-level instruction
information is the only thing we can't do through asmprinter. I'm open to
other ways of capturing the information that don't require comments in
MachineInstrs. But the information has to be stored somewhere.

Ok, lets deal with this as a separate pass. A good solution may be to have an on-the-side DenseMap<MachineInstr*,Instruction*>.

For now, lets go forward with the plan of having the asmprinter
generate the comments. It sounds like it can cover at least 75% of
what you're interested in, and the other cases probably have better
solutions that sticking a vector of std::string on MachineInstr.

That's ok by me. Let's hash through this some more as stuff comes on-line.

Thanks Dave!

-Chris

Yes, the whole reason this stuff exists is that I needed to improve
my quality of life at the time. :slight_smile:

                                -Dave

David Greene wrote:

I forgot about the LLVM instruction information. That's something that
can't be synthesized by the asmprinter. Again, we only emit some of this
with special debug flags so we don't carry the original IR around in
comments all the time. :slight_smile:

If the FI question can be answered (how do we know it's a spill slot)?
Then the above is the only use I think we don't have a long-term answer for.
I don't see how asmprinter can synthesize this information.

Line numbers are still a problem because we need to propagate the information
from LLVM IR to MachineInstrs and there's currently no DebugLoc information
in LLVM IR Instructions. I'll take a look at the existing DebugLoc and see if we can put our LLVM IR line number info there instead of printing it to comments early.

If it's any comfort, we've been running with this MachineInstr for a
very long time and done multiple public releases. We always go through
asm to do our x86 compiles and no one has ever complained about compile
time or memory usage in this area (legalize is actually our biggest problem). And we compile HUGE source files containing very large functions.

Of course, our requirements aren't the same as everyone else's requirements
so I understand that there may be tighter tolerances in some use cases. Examples of where the MachineInstr comments might be a problem would be helpful.

In JIT compilers anything that slows down compilation is BAD. Plus most VMs already have there own debug/introspection infrastructure, so none is required in the machine code.
Also when translating from a very high level language to llvm, there tends to be a *lot* of instructions, so keeping them small is important.

I have posted in the past lamenting llvm slow compilation and it has got faster since.

So, thanks to everyone for making the machine-code generation faster,
and please, keep making it faster :slight_smile:

In JIT compilers anything that slows down compilation is BAD. Plus most
VMs already have there own debug/introspection infrastructure, so none
is required in the machine code.

JITs don't generate asm.

Also when translating from a very high level language to llvm, there
tends to be a *lot* of instructions, so keeping them small is important.

Indeed. We've encountered very large functions here with no problem.

                                -Dave