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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

I'm just going through the remaining issues from Matt and I don't think there's anything that should block committing this. AFAICT there's only one issue still open and it's the one about reusing  the static printIRValueReference() in MachineOperand.cpp causing one function of MIRFormatter to be implemented in that file as well.



================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:484-488
+void MIRFormatter::printIRValue(raw_ostream &OS, const Value &V,
+                                ModuleSlotTracker &MST) {
+  printIRValueReference(OS, V, MST);
+}
+
----------------
pguo wrote:
> arsenm wrote:
> > I don't see the point of adding this function? This probably shouldn't be leaking the MIRPrinter into MachineOperand.cpp
> The idea is a helper function to print IR value in MIR serialization format, which can be used by target custom pseudo source value.  The target custom pseudo source value can hold IR value reference, it's better to print/parse it in a standard way.
I agree with printing/parsing it the same way but it is a bit odd to have part of the MIRFormatter implementation in this file. Is there somewhere that printIRValueReference() could be to make it available for both MachineOperand.cpp and a newly created MIRFormatter.cpp?

If not, I don't think it's a huge problem to have this function here. Both objects are implemented in the same library and I've seen this kind of thing elsewhere with some of the reader/writer implementations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69836