[MC] Remove unneeded MCSymbolRefExpr::create overload and add comments

The StringRef overload is often error-prone as users might forget to
register the MCSymbol.

Add comments to MCTargetExpr and MCSymbolRefExpr::VariantKind.
In the distant future the VariantKind parameter might be removed.

Author: MaskRay

llvm/include/llvm/MC/MCExpr.h

@@ -191,6 +191,11 @@ class MCConstantExpr : public MCExpr {
 /// of the symbol as external.
 class MCSymbolRefExpr : public MCExpr {
 public:
+  // VariantKind isn't ideal for encoding relocation operators because:
+  // (a) other expressions, like MCConstantExpr (e.g., 4@l) and MCBinaryExpr
+  // (e.g., (a+1)@l), also need it; (b) semantics become unclear (e.g., folding
+  // expressions with @). MCTargetExpr, as used by AArch64 and RISC-V, offers a
+  // cleaner approach.
   enum VariantKind : uint16_t {
     VK_None,
     VK_Invalid,
@@ -384,14 +389,13 @@ class MCSymbolRefExpr : public MCExpr {
   /// \name Construction
   /// @{
 
-  static const MCSymbolRefExpr *create(const MCSymbol *Symbol, MCContext &Ctx) {
-    return MCSymbolRefExpr::create(Symbol, VK_None, Ctx);
+  static const MCSymbolRefExpr *create(const MCSymbol *Symbol, MCContext &Ctx,
+                                       SMLoc Loc = SMLoc()) {
+    return MCSymbolRefExpr::create(Symbol, VK_None, Ctx, Loc);
   }
 
   static const MCSymbolRefExpr *create(const MCSymbol *Symbol, VariantKind Kind,
                                        MCContext &Ctx, SMLoc Loc = SMLoc());
-  static const MCSymbolRefExpr *create(StringRef Name, VariantKind Kind,
-                                       MCContext &Ctx);
 
   /// @}
   /// \name Accessors
@@ -630,8 +634,10 @@ class MCBinaryExpr : public MCExpr {
   }
 };
 
-/// This is an extension point for target-specific MCExpr subclasses to
-/// implement.
+/// Extension point for target-specific MCExpr subclasses to implement.
+/// This can encode a relocation operator, serving as a replacement for
+/// MCSymbolRefExpr::VariantKind. Ideally, limit this to
+/// top-level use, avoiding its inclusion as a subexpression.
 ///
 /// NOTE: All subclasses are required to have trivial destructors because
 /// MCExprs are bump pointer allocated and not destructed.

llvm/lib/MC/MCExpr.cpp

@@ -246,11 +246,6 @@ const MCSymbolRefExpr *MCSymbolRefExpr::create(const MCSymbol *Sym,
   return new (Ctx) MCSymbolRefExpr(Sym, Kind, Ctx.getAsmInfo(), Loc);
 }
 
-const MCSymbolRefExpr *MCSymbolRefExpr::create(StringRef Name, VariantKind Kind,
-                                               MCContext &Ctx) {
-  return create(Ctx.getOrCreateSymbol(Name), Kind, Ctx);
-}
-
 /* *** */
 
 void MCTargetExpr::anchor() {}

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

@@ -12438,9 +12438,9 @@ bool ARMAsmParser::parseDirectiveTLSDescSeq(SMLoc L) {
   if (getLexer().isNot(AsmToken::Identifier))
     return TokError("expected variable after '.tlsdescseq' directive");
 
-  const MCSymbolRefExpr *SRE =
-    MCSymbolRefExpr::create(Parser.getTok().getIdentifier(),
-                            MCSymbolRefExpr::VK_ARM_TLSDESCSEQ, getContext());
+  auto *Sym = getContext().getOrCreateSymbol(Parser.getTok().getIdentifier());
+  const auto *SRE = MCSymbolRefExpr::create(
+      Sym, MCSymbolRefExpr::VK_ARM_TLSDESCSEQ, getContext());
   Lex();
 
   if (parseEOL())

llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

@@ -1269,9 +1269,7 @@ void MipsTargetELFStreamer::emitDirectiveCpLoad(unsigned RegNo) {
   TmpInst.setOpcode(Mips::LUi);
   TmpInst.addOperand(MCOperand::createReg(GPReg));
   const MCExpr *HiSym = MipsMCExpr::create(
-      MipsMCExpr::MEK_HI,
-      MCSymbolRefExpr::create("_gp_disp", MCSymbolRefExpr::VK_None,
-                              MCA.getContext()),
+      MipsMCExpr::MEK_HI, MCSymbolRefExpr::create(GP_Disp, MCA.getContext()),
       MCA.getContext());
   TmpInst.addOperand(MCOperand::createExpr(HiSym));
   getStreamer().emitInstruction(TmpInst, STI);
@@ -1282,9 +1280,7 @@ void MipsTargetELFStreamer::emitDirectiveCpLoad(unsigned RegNo) {
   TmpInst.addOperand(MCOperand::createReg(GPReg));
   TmpInst.addOperand(MCOperand::createReg(GPReg));
   const MCExpr *LoSym = MipsMCExpr::create(
-      MipsMCExpr::MEK_LO,
-      MCSymbolRefExpr::create("_gp_disp", MCSymbolRefExpr::VK_None,
-                              MCA.getContext()),
+      MipsMCExpr::MEK_LO, MCSymbolRefExpr::create(GP_Disp, MCA.getContext()),
       MCA.getContext());
   TmpInst.addOperand(MCOperand::createExpr(LoSym));
   getStreamer().emitInstruction(TmpInst, STI);

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp

@@ -1539,8 +1539,8 @@ bool PPCAsmParser::parseOperand(OperandVector &Operands) {
       if (!(parseOptionalToken(AsmToken::Identifier) &&
             Tok.getString().compare_insensitive("plt") == 0))
         return Error(Tok.getLoc(), "expected 'plt'");
-      EVal = MCSymbolRefExpr::create(TlsGetAddr, MCSymbolRefExpr::VK_PLT,
-                                     getContext());
+      EVal = MCSymbolRefExpr::create(getContext().getOrCreateSymbol(TlsGetAddr),
+                                     MCSymbolRefExpr::VK_PLT, getContext());
       if (parseOptionalToken(AsmToken::Plus)) {
         const MCExpr *Addend = nullptr;
         SMLoc EndLoc;