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

[Peng Guo via Phabricator via llvm-commits]

pguo marked 2 inline comments as done.
pguo added inline comments.


================
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 {
----------------
dsanders wrote:
> 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)
Got it.  I switched to use Optional.  Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69836