[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 accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm



================
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);
 
----------------
chandlerc wrote:
> rnk wrote:
> > 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.
> I think it is useful for the caller to control the way the symbol is created.
> 
> For example, it might be specifically desirable for these to be external symbols for some passes.
> 
> The caller will still have to find all references to the current symbol and update them if it wants to change the symbol here, but it doesn't seem like the API should block that from happening just because it is hard?
> 
> Alternatively, I can make these be a *list* of symbols and emit all of them. That would make it even more usable for supporting different use cases for the symbols. Thoughts?
I think the external symbol use case is kind of interesting. Fair enough.


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:763
 
+  auto ParseInstrSymbol = [&](MIToken::TokenKind KW) -> Optional<MCSymbol *> {
+    if (!Token.is(KW))
----------------
chandlerc wrote:
> rnk wrote:
> > 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.
> Gah. Fine.
> 
> I really, really dislike this style which is why I didn't try to emulate it, but I also think you're right.
Thanks!


================
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>(
----------------
chandlerc wrote:
> rnk wrote:
> > 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.
> Folks seemed very concerned about the memory usage of `ExtraInfo` so I'm happy to keep it optimized for now... I can make them mutable though (or we can allow appending if that helps based on my other comment above). Probably a follow-up patch though?
Yeah, a follow-up is fine.


Repository:
  rL LLVM

https://reviews.llvm.org/D50833