This is an archive of the discontinued LLVM Phabricator instance.

[MIR][ARM] MachineOperand comments to print condition code names instead of constants
ClosedPublic

Authored by SjoerdMeijer on Feb 10 2020, 2:54 AM.

Details

Summary

Some machine operands have their values encoded as immediates and are thus printed as immediates, for example:

dead renamable $r2, $cpsr = tEOR killed renamable $r2, renamable $r1, 14, $noreg
t2Bcc %bb.4, 0, killed $cpsr

In this example, 14 is a condition code that means that the EOR instruction is not conditionally executed (it is "always executed"). The 0 on the next conditional branch instruction represents the equal condition code. The problem is that these constants make the MIR difficult to read (for a human), so here we propose to annotate MachineOperands with an annotation/comment:

dead renamable $r2, $cpsr = tEOR killed renamable $r2, renamable $r1, 14 [always], $noreg
t2Bcc %bb.4, 0 [equal], killed $cpsr

As it is a comment, the MI lexer/parser completely ignores it. The benefit is that this keeps the change in the lexer extremely minimal and no target specific parsing needs to be done. The changes on the MIPrinter side are also minimal, as targets hooks are used to create the machine operand comments.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Feb 10 2020, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2020, 2:54 AM

I think something like this is nice to have: various instructions use immediates with weird encodings that make perfect sense in the code, but are sort of hard to read.

I don't really like the way this is implemented; it's adding a bunch of target-specific logic to target-independent code.

I wonder if, instead of replacing the immediate with a keyword, it would make sense to add some generic mechanism to print a comments after an operand, to sort of explain it. We could print the condition for a condition code, but we could also decode various other kinds of encoded immediates: for example, "26" is "lsl #3" in ADDrsi. Printing it as a comment would be simpler to implement because the parsing change would be much smaller: we continue just parsing an integer, and ignore the comment, instead of adding target-specific parsing rules.

I think something like this is nice to have: various instructions use immediates with weird encodings that make perfect sense in the code, but are sort of hard to read.

Thanks for taking a look!

I don't really like the way this is implemented; it's adding a bunch of target-specific logic to target-independent code.

I could have tried harder to hide target specific logic in targets hooks in MIParse. In the MIRPrinter for example, I added a call to TII->getPredicateByName(). In the parser, for this WIP demo, I was lazy and just added getPredicateCondCode(), but could have done something similar. But I do fully agree of course that one way or another, that means more target specific parsing.

I wonder if, instead of replacing the immediate with a keyword, it would make sense to add some generic mechanism to print a comments after an operand, to sort of explain it. We could print the condition for a condition code, but we could also decode various other kinds of encoded immediates: for example, "26" is "lsl #3" in ADDrsi. Printing it as a comment would be simpler to implement because the parsing change would be much smaller: we continue just parsing an integer, and ignore the comment, instead of adding target-specific parsing rules.

Nice one, cheers, I do really like that idea! The only disadvantage I see is that one can argue that MIR output is possibly more verbose than needed, but all together this sounds like an excellent trade-off to me. I will give this a try and make a start with implementing this soon.

How about something like this?

This is roughly what I was expecting; I think this is the right approach.

Maybe getImmOperandComment should be a target-specific method, instead of putting the predicate handling logic in target-independent code? We probably want some general-purpose hook for adding comments anyway.

SjoerdMeijer retitled this revision from [MIR][ARM] WIP: Print condition code names instead of magic constants to [MIR][ARM] MachineOperand comments to print condition code names instead of constants.
SjoerdMeijer edited the summary of this revision. (Show Details)

Created createMIROperandComment() as general-purpose hook to create comments.

I've now updated all the tests, and so thought it was time to remove the WIP tag from the subject.

ostannard added inline comments.Feb 14 2020, 4:09 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1310

Is this still needed in the common TargetInstrInfo?

1313

Returning a const char * is going to make it hard for targets to dynamically construct comments, e.g. Eli's example of "26" -> "lsl #3" in ADDrsi. The MIR printer is only used for debugging, so using a std::string here should be fine.

llvm/lib/CodeGen/MIRParser/MILexer.cpp
107

Is this syntax used for comments anywhere else? It's not immediately obvious from reading the changed tests that this is a comment, could we use something more common like /* C-stlye */ comments?

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
500

I think it would be better to use the two-letter codes from the assembly syntax. They are more canonical, and easier to look up the exact meaning than these.

531

getPredicateByName can never return a null pointer.

Hi Oliver, thanks for looking too and the feedback!

Is this still needed in the common TargetInstrInfo?

Nope, good one, I have removed it. Now we only have createMIROperandComment() as a new target hook.

Returning a const char * is going to make it hard for targets

yep, done, that's a lot more convenient.

I think it would be better to use the two-letter codes from the assembly syntax. They are more canonical, and easier to look up the exact meaning than these.

Done, and related to this, I have completely removed getPredicateByName(), instead I can just call existing function ARMCondCodeToString().

Is this syntax used for comments anywhere else? It's not immediately obvious from reading the changed tests that this is a comment, could we use something more common like /* C-stlye */ comments?

I have no strong opinion on this. I chose [ and ] mainly for these 2 reasons:

  • it's a character currently not used by the lexer,
  • it is 1 character, which I found nice and concise and pleasant to read. I think I find it more pleasant to read than /* and */ in MIR code, but I appreciate this is very subjective, see next paragraph.

I have only added 1 changed test case, because the ~140 I had to change make things difficult to read here. I will make the final change changing all tests when we are happy about the exact syntax:

  • I have adopted the 2-letter codes syntax. To make this readable, I have prefixed this with CC:: to indicate this is a condition code, so that for example you'll see [CC::al].
  • As I said above, I have kept the [ and ] for now to open/close comments, but would be happy to change. If @efriedma has a preference, then I will take that into account, make the change, and then update all the tests.

About updating the tests, that's actually pretty straightforward because for about ~120 I can just use the script (just letting know that's how I changed them). For about 20 of them, that doesn't really work because of shared check-prefixes, so I need to change them "manually" with a little bit search/replace commands, but is not a big deal.

the same patch, but now with context.

I can see two problems with [] comment delimiters:

  1. ] is used somewhat frequently in target-specific assembly syntax, so it might end up overlapping with stuff some target wants to print. Maybe not that likely in practice.
  2. It isn't obvious that the contents of the [] is a comment. Something like /**/ would give instant recognition to people even if they're not really familiar with MIR syntax.

Otherwise looks fine.

Thanks again for the suggestions/comments. The MachineOperand comments are now enclosed between /* and */, see the only test file that I have uploaded.

As a last step, I can now change all tests. I can upload them here, or commit directly when you're happy with this patch. The only reason I have not uploaded the ~140 changed tests, is because that makes this Phab diff a bit unwieldy.

ostannard accepted this revision.Feb 24 2020, 2:12 AM

LGTM with one really minor nit.

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
501

Comments should end with full stops.

This revision is now accepted and ready to land.Feb 24 2020, 2:12 AM
This revision was automatically updated to reflect the committed changes.