[PATCH] D74306: [MIR][ARM] MachineOperand comments to print condition code names instead of constants

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer updated this revision to Diff 244639.
SjoerdMeijer added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74306/new/

https://reviews.llvm.org/D74306

Files:
  llvm/include/llvm/CodeGen/TargetInstrInfo.h
  llvm/lib/CodeGen/MIRParser/MILexer.cpp
  llvm/lib/CodeGen/MIRPrinter.cpp
  llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
  llvm/lib/Target/ARM/ARMBaseInstrInfo.h
  llvm/test/CodeGen/Thumb2/t2-teq-reduce.mir

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D74306.244639.patch
Type: text/x-patch
Size: 13184 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200214/e6896074/attachment.bin>