[PATCH] D69836: [MIR] Target specific MIR formating and parsing

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM from me.



================
Comment at: llvm/include/llvm/CodeGen/MIRFormatter.h:36
+  /// value of OpIdx is ~0 which means the index is unknown.
+  virtual void printImm(raw_ostream &OS, const MachineInstr &MI, unsigned OpIdx,
+                        int64_t Imm) const {
----------------
pguo wrote:
> dsanders wrote:
> > This is just an optional nitpick. Using ~0 is fine but if you used Optional<unsigned> then you wouldn't have to explain that. It does take a little more storage though so I don't mind if you stick to ~0
> I think the OpIdx here is not optional but required to get the correct functionally.  The tuple <OpCode, OpIdx> will uniquely tell formatter how to format the immediate in a target specific way.  While since we can do dump() on any MachineOperand object inside debugger, in this case we don't know the Index, so we have to pass ~0 to the real print function to mark it as unknown.
I think there may be some confusion here so just to clarify: Optional<unsigned> is essentially the same as what you're doing but the unknown case is more explicit and self documenting as it doesn't need a number to be carved out of `unsigned` and documented as being 'unknown':
  printImm(OS, MI, ~0 /* Unknown */, Imm)
becomes:
  printImm(OS, MI, None, Imm)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69836