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

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1310
+  /// Returns the name for the specified condition code
+  virtual const char *getPredicateByName(int CC) const {return nullptr; };
+
----------------
Is this still needed in the common TargetInstrInfo?


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1313
+  // Returns a MIRPrinter comment for this machine operand.
+  virtual const char *createMIROperandComment(const MachineInstr &MI,
+                                              const MachineOperand &Op,
----------------
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.


================
Comment at: llvm/lib/CodeGen/MIRParser/MILexer.cpp:107
 
+/// Machine operands can have comments, enclosed between '[' and ']'.
+/// This eats up all tokens, including the brackets.
----------------
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?


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:500
+  switch (CC) {
+  case 0:   return "equal";
+  case 1:   return "notequal";
----------------
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.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:531
+    return nullptr;
+  if (const char *PredName = getPredicateByName(Op.getImm()))
+    return PredName;
----------------
`getPredicateByName` can never return a null pointer.


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

https://reviews.llvm.org/D74306