[PATCH] D50833: [x86/MIR] Implement support for pre- and post-instruction symbols, as well as MIR parsing support for `MCSymbol` `MachineOperand`s.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1501-1502
   ///
   /// FIXME: This is not fully implemented yet.
-  MCSymbol *getOrCreatePreInstrTempSymbol(MCContext &MCCtx);
+  void setPreInstrSymbol(MachineFunction &MF, MCSymbol *Symbol);
 
----------------
I still think getOrCreate is the right API for CodeGen. Should this be made private between MachineInstr and MIParser? The only way I can imagine to do that is to introduce some public MIParserHelper class which we forward declare and then define only in the MIParser.cpp file.


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:763
 
+  auto ParseInstrSymbol = [&](MIToken::TokenKind KW) -> Optional<MCSymbol *> {
+    if (!Token.is(KW))
----------------
I think it would be more stylistically consistent for this to be a method of MIParser. Something just like `parseMachineOperandAndTiedFlags` above. I'd also use out parameters to match the error handling style.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:507
+  // Otherwise, allocate a full new set of extra info.
+  // FIXME: Maybe we should make the symbols in the extra info mutable?
   Info.set<EIIK_OutOfLine>(
----------------
That should be fairly easy if every MachineInstr owns its own ExtraInfo. Also, I don't think we're saving much memory from using TrailingObjects for the optional pre and post labels. We might as well make those regular nullable pointer members of ExtraInfo for simplicity. Then, mutating them is pretty straightforward.


Repository:
  rL LLVM

https://reviews.llvm.org/D50833