[X86][MS-InlineAsm] Extended support for variables / identifiers on memory / immediate expressions

[coby]

Summary

Allow the proper recognition of Enum values and global variables inside ms inline-asm memory / immediate expressions, as they require some additional overhead and treated incorrect if doesn't early recognized.
supersedes D33278, D35774

[RKSimon]

include/llvm/MC/MCParser/MCAsmParser.h

43 ↗

(On Diff #113658)

Preferred enum style is

enum IdKind {
   IK_Invalid,      // Initial state. Unexpected after a successful parsing.
   IK_Label,    // Function/Label reference.
   IK_EnumVal,  // Value of enumration type.
   IK_Var       // Variable.
 };

lib/Target/X86/AsmParser/X86AsmParser.cpp

589 ↗

(On Diff #113658)

if (IDInfo.isKind(InlineAsmIdentifierInfo::EnumValKind))
  return onInteger(IDInfo.Enum.EnumVal, ErrMsg);

595 ↗

(On Diff #113658)

// Treat a symbolic constant like an integer
if (auto *CE = dyn_cast<MCConstantExpr>(SymRef))
  return onInteger(CE->getValue(), ErrMsg);

1275 ↗

(On Diff #113658)

This looks like its a NFC commit that can be done separately

[coby]

include/llvm/MC/MCParser/MCAsmParser.h

43 ↗

(On Diff #113658)

accepted

lib/Target/X86/AsmParser/X86AsmParser.cpp

1275 ↗

(On Diff #113658)

can you be a bit more specific regarding the exact part which can be commited as a NFC?
The only one i'm trivially spotting is the encapsulation of the constant symbolic reference inside of 'onIdentifierExpr', when parsing assembly (non inline-asm) content, rather than prior calling it - was that the issue you've meant?

[RKSimon]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1275 ↗

(On Diff #113658)

I meant const InlineAsmIdentifierInfo &Info could be committed right now - it's only ever used as a const already.

[coby]

Addressed Simon's comments

[rnk]

Looks good

[RKSimon]

LGTM as well, with a couple of minors.

lib/Target/X86/AsmParser/X86AsmParser.cpp

589 ↗

(On Diff #113658)

This tidyup would still be nice.

595 ↗

(On Diff #113658)

This tidyup would still be nice.

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

589 ↗

(On Diff #113658)

sorry, don't know why've i missed this one out
will be added to the commit
thanks

595 ↗

(On Diff #113658)

same

[coby]

also, i'll appreciate if you may review/accept the (essential) frontend changes - D37413

llvm/trunk/include/llvm/MC/MCParser/MCAsmParser.h

 class MCContext;
 class MCExpr;
 class MCInstPrinter;
 class MCInstrInfo;
 class MCStreamer;
 class MCTargetAsmParser;
 class SourceMgr;
 
-class InlineAsmIdentifierInfo {
-public:
-  void *OpDecl;
-  bool IsVarDecl;
-  unsigned Length, Size, Type;
-
-  void clear() {
-    OpDecl = nullptr;
-    IsVarDecl = false;
-    Length = 1;
-    Size = 0;
-    Type = 0;
+struct InlineAsmIdentifierInfo {
+  enum IdKind {
+    IK_Invalid,  // Initial state. Unexpected after a successful parsing.
+    IK_Label,    // Function/Label reference.
+    IK_EnumVal,  // Value of enumration type.
+    IK_Var       // Variable.
+  };
+  // Represents an Enum value
+  struct EnumIdentifier {
+    int64_t EnumVal;
+  };
+  // Represents a label/function reference
+  struct LabelIdentifier {
+    void *Decl;
+  };
+  // Represents a variable
+  struct VariableIdentifier {
+    void *Decl;
+    bool IsGlobalLV;
+    unsigned Length;
+    unsigned Size;
+    unsigned Type;
+  };
+  // An InlineAsm identifier can only be one of those
+  union {
+    EnumIdentifier Enum;
+    LabelIdentifier Label;
+    VariableIdentifier Var;
+  };
+  bool isKind(IdKind kind) const { return Kind == kind; }
+  // Initializers
+  void setEnum(int64_t enumVal) {
+    assert(isKind(IK_Invalid) && "should be initialized only once");
+    Kind = IK_EnumVal;
+    Enum.EnumVal = enumVal;
+  }
+  void setLabel(void *decl) {
+    assert(isKind(IK_Invalid) && "should be initialized only once");
+    Kind = IK_Label;
+    Label.Decl = decl;
   }
+  void setVar(void *decl, bool isGlobalLV, unsigned size, unsigned type) {
+    assert(isKind(IK_Invalid) && "should be initialized only once");
+    Kind = IK_Var;
+    Var.Decl = decl;
+    Var.IsGlobalLV = isGlobalLV;
+    Var.Size = size;
+    Var.Type = type;
+    Var.Length = size / type;
+  }
+  InlineAsmIdentifierInfo() : Kind(IK_Invalid) {}
+private:
+  // Discrimint using the current kind
+  IdKind Kind;
 };
 
 /// \brief Generic Sema callback for assembly parser.
 class MCAsmParserSemaCallback {
 public:
   virtual ~MCAsmParserSemaCallback();
 
-  virtual void *LookupInlineAsmIdentifier(StringRef &LineBuf,
-                                          InlineAsmIdentifierInfo &Info,
-                                          bool IsUnevaluatedContext) = 0;
+  virtual void LookupInlineAsmIdentifier(StringRef &LineBuf,
   virtual StringRef LookupInlineAsmLabel(StringRef Identifier, SourceMgr &SM,
                                          SMLoc Location, bool Create) = 0;
   virtual bool LookupInlineAsmField(StringRef Base, StringRef Member,
                                     unsigned &Offset) = 0;
 };
 
 /// \brief Generic assembler parser interface, for use by target specific
 /// assembly parsers.

llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp

class IntelExprStateMachine {
     InlineAsmIdentifierInfo Info;
     short BracCount;
     bool MemExpr;
 
   public:
     IntelExprStateMachine()
         : State(IES_INIT), PrevState(IES_ERROR), BaseReg(0), IndexReg(0),
           TmpReg(0), Scale(1), Imm(0), Sym(nullptr), BracCount(0),
-          MemExpr(false) {
-      Info.clear();
-    }
+          MemExpr(false) {}
 
     void addImm(int64_t imm) { Imm += imm; }
     short getBracCount() { return BracCount; }
     bool isMemExpr() { return MemExpr; }
     unsigned getBaseReg() { return BaseReg; }
     unsigned getIndexReg() { return IndexReg; }
     unsigned getScale() { return Scale; }
     const MCExpr *getSym() { return Sym; }
bool onRegister(unsigned Reg, StringRef &ErrMsg) {
           State = IES_ERROR;
         }
         break;
       }
       PrevState = CurrState;
       return false;
     }
     bool onIdentifierExpr(const MCExpr *SymRef, StringRef SymRefName,
-                          StringRef &ErrMsg) {
+                          const InlineAsmIdentifierInfo &IDInfo,
+                          bool ParsingInlineAsm, StringRef &ErrMsg) {
+      // InlineAsm: Treat an enum value as an integer
+      if (ParsingInlineAsm)
+        if (IDInfo.isKind(InlineAsmIdentifierInfo::IK_EnumVal))
+          return onInteger(IDInfo.Enum.EnumVal, ErrMsg);
+      // Treat a symbolic constant like an integer
+      if (auto *CE = dyn_cast<MCConstantExpr>(SymRef))
+        return onInteger(CE->getValue(), ErrMsg);
       PrevState = State;
       bool HasSymbol = Sym != nullptr;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_PLUS:
       case IES_MINUS:
       case IES_NOT:
       case IES_INIT:
       case IES_LBRAC:
-        MemExpr = !(SymRef->getKind() == MCExpr::Constant);
+        MemExpr = true;
         State = IES_INTEGER;
         Sym = SymRef;
         SymName = SymRefName;
         IC.pushOperand(IC_IMM);
+        if (ParsingInlineAsm)
+          Info = IDInfo;
         break;
       }
       if (HasSymbol)
         ErrMsg = "cannot use more than one symbol in memory operand";
       return HasSymbol;
     }
     bool onInteger(int64_t TmpInt, StringRef &ErrMsg) {
       IntelExprState CurrState = State;

 }
 
 std::unique_ptr<X86Operand> X86AsmParser::CreateMemForInlineAsm(
     unsigned SegReg, const MCExpr *Disp, unsigned BaseReg, unsigned IndexReg,
     unsigned Scale, SMLoc Start, SMLoc End, unsigned Size, StringRef Identifier,
     const InlineAsmIdentifierInfo &Info) {
   // If we found a decl other than a VarDecl, then assume it is a FuncDecl or
   // some other label reference.
-  if (isa<MCSymbolRefExpr>(Disp) && Info.OpDecl && !Info.IsVarDecl) {
+  if (Info.isKind(InlineAsmIdentifierInfo::IK_Label)) {
     // Insert an explicit size if the user didn't have one.
     if (!Size) {
       Size = getPointerWidth();
       InstInfo->AsmRewrites->emplace_back(AOK_SizeDirective, Start,
                                           /*Len=*/0, Size);
     }
-
     // Create an absolute memory reference in order to match against
     // instructions taking a PC relative operand.
     return X86Operand::CreateMem(getPointerWidth(), Disp, Start, End, Size,
-                                 Identifier, Info.OpDecl);
+                                 Identifier, Info.Label.Decl);
   }
-
-
   // We either have a direct symbol reference, or an offset from a symbol.  The
   // parser always puts the symbol on the LHS, so look there for size
   // calculation purposes.
   unsigned FrontendSize = 0;
-  const MCBinaryExpr *BinOp = dyn_cast<MCBinaryExpr>(Disp);
-  bool IsSymRef =
-      isa<MCSymbolRefExpr>(BinOp ? BinOp->getLHS() : Disp);
-  if (IsSymRef && !Size && Info.Type)
-    FrontendSize = Info.Type * 8; // Size is in terms of bits in this context.
-
-  // When parsing inline assembly we set the base register to a non-zero value
+  void *Decl = nullptr;
+  bool IsGlobalLV = false;
+  if (Info.isKind(InlineAsmIdentifierInfo::IK_Var)) {
+    // Size is in terms of bits in this context.
+    FrontendSize = Info.Var.Type * 8;
+    Decl = Info.Var.Decl;
+    IsGlobalLV = Info.Var.IsGlobalLV;
+  }
+  // It is widely common for MS InlineAsm to use a global variable and one/two
+  // registers in a mmory expression, and though unaccessible via rip/eip.
+  if (IsGlobalLV && (BaseReg || IndexReg)) {
+    return X86Operand::CreateMem(getPointerWidth(), Disp, Start, End);
+  // Otherwise, we set the base register to a non-zero value
   // if we don't know the actual value at this time.  This is necessary to
   // get the matching correct in some cases.
+  } else {
     BaseReg = BaseReg ? BaseReg : 1;
     return X86Operand::CreateMem(getPointerWidth(), SegReg, Disp, BaseReg,
                                  IndexReg, Scale, Start, End, Size, Identifier,
-                               Info.OpDecl, FrontendSize);
+                                 Decl, FrontendSize);
+  }
 }
 
 // Some binary bitwise operators have a named synonymous
 // Query a candidate string for being such a named operator
 // and if so - invoke the appropriate handler
 bool X86AsmParser::ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine &SM) {
   // A named operator should be either lower or upper case, but not a mix
   if (Name.compare(Name.lower()) && Name.compare(Name.upper()))
while (!Done) {
     case AsmToken::Real:
       // DotOperator: [ebx].0
       UpdateLocLex = false;
       if (ParseIntelDotOperator(SM, End))
         return true;
       break;
     case AsmToken::String:
     case AsmToken::Identifier: {
-      // This could be a register or a symbolic displacement.
-      unsigned TmpReg;
-      const MCExpr *Val;
       SMLoc IdentLoc = Tok.getLoc();
       StringRef Identifier = Tok.getString();
       UpdateLocLex = false;
-      if (TK != AsmToken::String && !ParseRegister(TmpReg, IdentLoc, End)) {
-        if (SM.onRegister(TmpReg, ErrMsg))
+      // Register
+      unsigned Reg;
+      if (Tok.isNot(AsmToken::String) && !ParseRegister(Reg, IdentLoc, End)) {
+        if (SM.onRegister(Reg, ErrMsg))
           return Error(Tok.getLoc(), ErrMsg);
-      } else if (ParseIntelNamedOperator(Identifier, SM)) {
-        UpdateLocLex = true;
-      } else if (!isParsingInlineAsm()) {
-        if (getParser().parsePrimaryExpr(Val, End))
+        break;
+      }
+      // Operator synonymous ("not", "or" etc.)
+      if ((UpdateLocLex = ParseIntelNamedOperator(Identifier, SM)))
+        break;
+      // Symbol reference, when parsing assembly content
+      InlineAsmIdentifierInfo Info;
+      const MCExpr *Val;
+      if (!isParsingInlineAsm()) {
+        if (getParser().parsePrimaryExpr(Val, End)) {
           return Error(Tok.getLoc(), "Unexpected identifier!");
-        if (auto *CE = dyn_cast<MCConstantExpr>(Val)) {
-          if (SM.onInteger(CE->getValue(), ErrMsg))
-            return Error(IdentLoc, ErrMsg);
-        } else if (SM.onIdentifierExpr(Val, Identifier, ErrMsg))
+        } else if (SM.onIdentifierExpr(Val, Identifier, Info, false, ErrMsg)) {
           return Error(IdentLoc, ErrMsg);
-      } else if (unsigned OpKind = IdentifyIntelInlineAsmOperator(Identifier)) {
+        } else
+          break;
+      }
+      // MS InlineAsm operators (TYPE/LENGTH/SIZE)
+      if (unsigned OpKind = IdentifyIntelInlineAsmOperator(Identifier)) {
         if (OpKind == IOK_OFFSET)
           return Error(IdentLoc, "Dealing OFFSET operator as part of"
             "a compound immediate expression is yet to be supported");
-        int64_t Val = ParseIntelInlineAsmOperator(OpKind);
-        if (!Val)
-          return true;
+        if (int64_t Val = ParseIntelInlineAsmOperator(OpKind)) {
           if (SM.onInteger(Val, ErrMsg))
             return Error(IdentLoc, ErrMsg);
-      } else if (Identifier.count('.') && PrevTK == AsmToken::RBrac) {
-          if (ParseIntelDotOperator(SM, End))
-            return true;
-      } else if (ParseIntelInlineAsmIdentifier(Val, Identifier,
-                                               SM.getIdentifierInfo(),
-                                               /*Unevaluated=*/false, End)) {
+        } else
+          return true;
+        break;
+      }
+      // MS Dot Operator expression
+      if (Identifier.count('.') && PrevTK == AsmToken::RBrac) {
+        break;
+      }
+      // MS InlineAsm identifier
+      if (ParseIntelInlineAsmIdentifier(Val, Identifier, Info, false, End))
         return true;
-      } else if (SM.onIdentifierExpr(Val, Identifier, ErrMsg)) {
+      else if (SM.onIdentifierExpr(Val, Identifier, Info, true, ErrMsg))
         return Error(IdentLoc, ErrMsg);
-      }
       break;
     }
     case AsmToken::Integer: {
       // Look for 'b' or 'f' following an Integer as a directional label
       SMLoc Loc = getTok().getLoc();
       int64_t IntVal = getTok().getIntVal();
       End = consumeToken();
       UpdateLocLex = false;
       if (getLexer().getKind() == AsmToken::Identifier) {
         StringRef IDVal = getTok().getString();
         if (IDVal == "f" || IDVal == "b") {
           MCSymbol *Sym =
               getContext().getDirectionalLocalSymbol(IntVal, IDVal == "b");
           MCSymbolRefExpr::VariantKind Variant = MCSymbolRefExpr::VK_None;
           const MCExpr *Val =
               MCSymbolRefExpr::create(Sym, Variant, getContext());
           if (IDVal == "b" && Sym->isUndefined())
             return Error(Loc, "invalid reference to undefined symbol");
           StringRef Identifier = Sym->getName();
-          if (SM.onIdentifierExpr(Val, Identifier, ErrMsg))
+          InlineAsmIdentifierInfo Info;
+          if (SM.onIdentifierExpr(Val, Identifier, Info,
+              isParsingInlineAsm(), ErrMsg))
             return Error(Loc, ErrMsg);
           End = consumeToken();
         } else {
           if (SM.onInteger(IntVal, ErrMsg))
             return Error(Loc, ErrMsg);
         }
       } else {
         if (SM.onInteger(IntVal, ErrMsg))
bool X86AsmParser::ParseIntelInlineAsmIdentifier(const MCExpr *&Val,
                                                  InlineAsmIdentifierInfo &Info,
                                                  bool IsUnevaluatedOperand,
                                                  SMLoc &End) {
   MCAsmParser &Parser = getParser();
   assert(isParsingInlineAsm() && "Expected to be parsing inline assembly.");
   Val = nullptr;
 
   StringRef LineBuf(Identifier.data());
-  void *Result =
-    SemaCallback->LookupInlineAsmIdentifier(LineBuf, Info, IsUnevaluatedOperand);
 
   const AsmToken &Tok = Parser.getTok();
   SMLoc Loc = Tok.getLoc();
 
   // Advance the token stream until the end of the current token is
   // after the end of what the frontend claimed.
   const char *EndPtr = Tok.getLoc().getPointer() + LineBuf.size();
   do {
     End = Tok.getEndLoc();
     getLexer().Lex();
   } while (End.getPointer() < EndPtr);
   Identifier = LineBuf;
 
   // The frontend should end parsing on an assembler token boundary, unless it
   // failed parsing.
-  assert((End.getPointer() == EndPtr || !Result) &&
+  assert((End.getPointer() == EndPtr ||
+          Info.isKind(InlineAsmIdentifierInfo::IK_Invalid)) &&
           "frontend claimed part of a token?");
 
   // If the identifier lookup was unsuccessful, assume that we are dealing with
   // a label.
-  if (!Result) {
+  if (Info.isKind(InlineAsmIdentifierInfo::IK_Invalid)) {
     StringRef InternalName =
       SemaCallback->LookupInlineAsmLabel(Identifier, getSourceManager(),
                                          Loc, false);
     assert(InternalName.size() && "We should have an internal name here.");
     // Push a rewrite for replacing the identifier name with the internal name.
     InstInfo->AsmRewrites->emplace_back(AOK_Label, Loc, Identifier.size(),
                                         InternalName);
-  }
-
+  } else if (Info.isKind(InlineAsmIdentifierInfo::IK_EnumVal))
+    return false;
   // Create the symbol reference.
   MCSymbol *Sym = getContext().getOrCreateSymbol(Identifier);
   MCSymbolRefExpr::VariantKind Variant = MCSymbolRefExpr::VK_None;
   Val = MCSymbolRefExpr::create(Sym, Variant, getParser().getContext());
   return false;
 }
 
 //ParseRoundingModeOp - Parse AVX-512 rounding mode operand
std::unique_ptr<X86Operand> X86AsmParser::ParseIntelOffsetOfOperator() {
   const MCExpr *Val;
   InlineAsmIdentifierInfo Info;
   SMLoc Start = Tok.getLoc(), End;
   StringRef Identifier = Tok.getString();
   if (ParseIntelInlineAsmIdentifier(Val, Identifier, Info,
                                     /*Unevaluated=*/false, End))
     return nullptr;
 
+  void *Decl = nullptr;
+  // FIXME: MS evaluates "offset <Constant>" to the underlying integral
+  if (Info.isKind(InlineAsmIdentifierInfo::IK_EnumVal))
+    return ErrorOperand(Start, "offset operator cannot yet handle constants");
+  else if (Info.isKind(InlineAsmIdentifierInfo::IK_Var))
+    Decl = Info.Var.Decl;
   // Don't emit the offset operator.
   InstInfo->AsmRewrites->emplace_back(AOK_Skip, OffsetOfLoc, 7);
 
   // The offset operator will have an 'r' constraint, thus we need to create
   // register operand to ensure proper matching.  Just pick a GPR based on
   // the size of a pointer.
   bool Parse32 = is32BitMode() || Code16GCC;
   unsigned RegNo = is64BitMode() ? X86::RBX : (Parse32 ? X86::EBX : X86::BX);
 
   return X86Operand::CreateReg(RegNo, Start, End, /*GetAddress=*/true,
-                               OffsetOfLoc, Identifier, Info.OpDecl);
+                               OffsetOfLoc, Identifier, Decl);
 }
 
 // Query a candidate string for being an Intel assembly operator
 // Report back its kind, or IOK_INVALID if does not evaluated as a known one
 unsigned X86AsmParser::IdentifyIntelInlineAsmOperator(StringRef Name) {
   return StringSwitch<unsigned>(Name)
     .Cases("TYPE","type",IOK_TYPE)
     .Cases("SIZE","size",IOK_SIZE)
unsigned X86AsmParser::ParseIntelInlineAsmOperator(unsigned OpKind) {
   const MCExpr *Val = nullptr;
   InlineAsmIdentifierInfo Info;
   SMLoc Start = Tok.getLoc(), End;
   StringRef Identifier = Tok.getString();
   if (ParseIntelInlineAsmIdentifier(Val, Identifier, Info,
                                     /*Unevaluated=*/true, End))
     return 0;
 
-  if (!Info.OpDecl) {
+  if (!Info.isKind(InlineAsmIdentifierInfo::IK_Var)) {
     Error(Start, "unable to lookup expression");
     return 0;
   }
 
   unsigned CVal = 0;
   switch(OpKind) {
   default: llvm_unreachable("Unexpected operand kind!");
-  case IOK_LENGTH: CVal = Info.Length; break;
-  case IOK_SIZE: CVal = Info.Size; break;
-  case IOK_TYPE: CVal = Info.Type; break;
+  case IOK_LENGTH: CVal = Info.Var.Length; break;
+  case IOK_SIZE: CVal = Info.Var.Size; break;
+  case IOK_TYPE: CVal = Info.Var.Type; break;
   }
 
   return CVal;
 }
 
 bool X86AsmParser::ParseIntelMemoryOperandSize(unsigned &Size) {
   Size = StringSwitch<unsigned>(getTok().getString())
     .Cases("BYTE", "byte", 8)