adding comment

Is there a simple way to add a comment in the machine instructions of a basic block?

Ideally something that can be used with machine instruction builder.

Tia.

Reed

In this case, I am working on the fast instruction selector for mips.

I want to be able to tag instructions or regions of instructions that are emitted by the fast instruction selector as opposed to the normal mechanism.

In fast instruction selector can always report that it cannot emit instructions for some construct and then that part falls on the normal mechanism; though in some cases the actual machine instructions emitted would be the same.

So I want to be able to look for some kind of tag in the make check tests.

Tia.

Reed

Would adding a flag to MachineInstr::MIFlag do the trick? I'm thinking that fast isel could ensure that a flag (e.g. MIFlag::FastISel) is added to the instructions it creates, then the instruction printer could optionally emit a comment for instructions that have this flag.

Maybe adding it to CommentFlag would be slightly better.

I’ve also been looking for something like this. I’m trying to start work on a pass that would be easiest to implement by merging multiple machine basic blocks into one, but I would really like to retain the comments marking the beginning of the merged blocks. I’ve been considering more complicated ways of doing it rather than losing those

-Matt

There is definitely a need for a mechanism to be able to add character data to any instruction which will get added as a comment.

Reed

Is there a simple way to add a comment in the machine instructions of a basic block?

Ideally something that can be used with machine instruction builder.

I’ve also been looking for something like this. I’m trying to start work on a pass that would be easiest to implement by merging multiple machine basic blocks into one, but I would really like to retain the comments marking the beginning of the merged blocks. I’ve been considering more complicated ways of doing it rather than losing those

-Matt

There is definitely a need for a mechanism to be able to add character data to any instruction which will get added as a comment.

You could do this now with MachineInstrBuilder.addMetadata(…)

Its not the nicest solution, but if you put an MDString inside the metadata then you can attach anything you want to an MI. Then you’d just need the asm printer to understand these. It somewhat goes against the spirit of codegen not creating IR, but it would work.

Thanks,
Pete

Is there a simple way to add a comment in the machine instructions of a basic block?

Ideally something that can be used with machine instruction builder.

I’ve also been looking for something like this. I’m trying to start work on a pass that would be easiest to implement by merging multiple machine basic blocks into one, but I would really like to retain the comments marking the beginning of the merged blocks. I’ve been considering more complicated ways of doing it rather than losing those

-Matt

There is definitely a need for a mechanism to be able to add character data to any instruction which will get added as a comment.

You could do this now with MachineInstrBuilder.addMetadata(…)

Its not the nicest solution, but if you put an MDString inside the metadata then you can attach anything you want to an MI. Then you’d just need the asm printer to understand these. It somewhat goes against the spirit of codegen not creating IR, but it would work.

Are you sure that you can even do that?

IR may appear as a constant at this time and place in the compilation.

I tried out CommentFlag and it looks ideal for this purpose. The attached patch adds a FastISel flag and prints it in the AsmPrinter and the IR dump. You can then call MachineInstr::setAsmPrinterFlag(MachineInstr::FastISel) to set the flag.

Some passes seem to be stripping the comment flags though. The one I noticed is the pseudo-instruction expansion pass:
  # *** IR Dump After Machine Copy Propagation Pass ***:
  # Machine code for function retfloat: Post SSA

  BB#0: derived from LLVM BB %entry
    RetRA; comment-flags: FastISel

  # End machine code for function retfloat.

  # *** IR Dump After Post-RA pseudo instruction expansion pass ***:
  # Machine code for function retfloat: Post SSA

  BB#0: derived from LLVM BB %entry
    RET %RA

  # End machine code for function retfloat.

comment-flags.patch (1.61 KB)

Nice. I suggest you submit the patch to llvm commits.

That will make more realistic make check tests for fast-isel and make it easy to see which code is emitted by fast-isel
and normal isel.

We've generally used -fast-isel-verbose and -fast-isel-abort to deal
with a lot of the testing of fast-isel these days, but if you can show
a compelling use case for testing where you can't write tests
otherwise then being able to increase our test coverage via other
means is always welcome.

-eric

Okay.

I did not know about that feature. That is definitely helpful.
You can surely verify that IR has been matched or not.

It's still useful to match patterns of asm code that you know are emitted by fast-isel and to not
confuse that with bits from normal instruction selection processing.

Reed

There's two that I can think of. The first is that it allows you to take an existing test case and incrementally support it in FastISel by adding checks for 'FastISel'. Once everything is covered you'd switch it over to use -fast-isel-abort. Combined with a way to initialize variables in FileCheck, I think this would be quite a convenient way to improve the coverage of FastISel with minimal changes to the tests. An example test might have the following checks:
  CHECK-LET: [[FastISel:]]
  FASTISEL-LET: [[FastISel:; comment-flag: FastISel]]
  CHECK: mul $2, $3, $4
  CHECK: add $2, $2, $3 [[FASTISEL]]
  CHECK: jr $ra [[FASTISEL]]
The RUN with -fast-isel enabled would process the CHECK and FASTISEL prefixes, the one with it disabled would only process the CHECK prefix.

The second is very similar but you never switch over to -fast-isel-abort. At the moment, there doesn't seem to be a way to test that the fall-back to SelectionDAG works. Admittedly, this gets less important (and harder to test) as FastISel becomes more complete but it would be good to have a couple tests for the fallback.

By the way, -fast-isel-abort doesn't abort when a terminator falls fails to select code.

I tried out CommentFlag and it looks ideal for this purpose. The attached patch adds a FastISel flag and prints it in the AsmPrinter and the IR dump. You can then call MachineInstr::setAsmPrinterFlag(MachineInstr::FastISel) to set the flag.

Some passes seem to be stripping the comment flags though. The one I noticed is the pseudo-instruction expansion pass:

Right. Flags is not something which one would expect to be accurate
and preserved. The MI passes are allowed to remove / alter the flags
in any way.