[LLVM][x86][Inline Asm] - Enum support for MS syntax

[mharoush]

Summary

This patch enables the usage of constant Enum identifiers within Microsoft style inline assembly statements.

part 2 out of 2.
[D33277]

[rnk]

Please consider using the monorepo to upload and submit this as a single patch.

include/llvm/MC/MCParser/MCAsmParser.h

77

It would be cleaner if InlineAsmIdentifierInfo simply contained an APInt indicating that the identifier in question was an enum, or other integral constant expression. Less callbacks to implement.

lib/Target/X86/AsmParser/X86AsmParser.cpp

722

Please try to eliminate the need for this extra boolean out parameter.

[mharoush]

Using identifier info to pass enum information.

[mharoush]

include/llvm/MC/MCParser/MCAsmParser.h

77

Done

lib/Target/X86/AsmParser/X86AsmParser.cpp

722

This flag is used in case we try to compile something like mov edx, A+6 ( when A is defined as ConstantEnum ) the original condition (Line:1905) will skip the rewrite since we have an identifier as the first Token, however if we try to compile 6+A it will be rewritten as expected.

I tried to solve this in different ways, I got either the exact opposite (A+6 was replaced and 6+A wasn't) or a collision of rewrites when trying to preform other forms of replacements and leaving this section intact.

I can perhaps change the way we handle general expressions to the same way we handle them under parseIntelBracExpression, meaning use the first iteration of X86AsmParser to reformat the string (write imm prefixes and replace identifiers when needed) then refactor the string to its canonical form on the second pass.

In short this was the simplest solution that worked without regression, maybe you have a better idea?

If the issue is the method signature pollution I can move this flag into the SM class as a member to indicate a rewrite is needed, however since this flag and method are limited to this context alone, I am not sure if the overhead is wanted in such case .

[rnk]

include/llvm/MC/MCParser/MCAsmParser.h

47–48

Please group the booleans to reduce padding. Ideally, make this an enum something like:

enum IdKind : uint8_t {
  Variable,
  Function,
  ConstInt,
};
IdKind Kind;

This is smaller and prevents nonsense states.

77

Thanks! This is better.

lib/Target/X86/AsmParser/X86AsmParser.cpp

722

I suspect there is a way to simplify the code so that this corner case no longer exists. I don't really have time to dig through the test cases to come up with it, but please make an effort.

1379

Please use clang-format here and elsewhere

[mharoush]

Simplified the AsmRewrite condition in x86AsmParser. Some fixes to simplify the Intel State Machine immediate value rewrite treatment divergence.

[mharoush]

lib/Target/X86/AsmParser/X86AsmParser.cpp

722

I believe this should resolve the complex if statement, However I still need the boolean value to enforce the rewrite for enum identifiers.

[mharoush]

ping

[mharoush]

ping

[rnk]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1379

not addressed?

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1310

blank line omitted

1376

assumption ~~> assertion

1378

I think this whole section better suites within its own function. something like 'ParseInlineAsmEnumValue'

1381

rephrase

1383

clang format

1386

'isParsingInlineAsm()' is unnecessary here (you can only reach this piece of code when parsing inline asm)

1823–1833

Not keen to the use of SM.getAddImmPrefix() as a mean of distinguish whether we are parsing a bracketed expression. I know it is currently turned on when parsing it, but it isn't asserted/guaranteed.
Regardless - I'm pretty sure we can manage without this rewrite, or at the very least - should, now that TYPE/LENGTH/SIZE are part of the State Machine.

1905–1906

you may just perform an AOK_Imm rewrite regardless the complexity of the immediate expression, and neglect 'ReplaceEnumIdentifier'

[mharoush]

simplified the rewrite condition of complex expressions and eliminated the need to use the ReplaceEnumIdentifier flag. This requires making small adjustments to some of the older test cases seen in [D33277]. Made some minor alterations and clarifications to satisfy reviewer comments.

[mharoush]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1376

I can assert the Identifier Info is indeed an enum. However, this is a general handle for any kind of value that may by passed back from the identifier parsing. There is no point in this assertion here since the value of type MCConstantExper is always safe. We simply make the decision at the parse identifier method.

1378

not much added value here and only a few lines can be moved (~5). Its not really worth it.

1379

Persistent text wrapping of my editor, fixed.

1381

Your comment is too general, I made some clarifications.

1386

This is just for readability and consistency with SM.onInteger(). Replaced with assertion to ease your mind

1823–1833

I meant this flag is only set when parsing bracketed expressions. However, I will rephrase the comment since we only care about the AddImmPrefix flag, which reflects the SM matching flag. The objective here is to match the behavior required on integer constants when the SM flag is set.

1905–1906

Thanks, this seems fine after the condition refinement. Just had to change some of the older inline asm tests which expects the IR statement to contain a binary/octal/hex base immediate value which this rewrite will replace with the decimal value.

[mharoush]

ping

[mharoush]

Cosmetics + ping

[rnk]

lgtm

Thanks @coby for finding the simplification!

lib/Target/X86/AsmParser/X86AsmParser.cpp

1397–1398

clang-format this

1905–1906

Nice. :)

[chapuni]

FYI, rL308966 has CRLF-ended paragraph in ParseIntelExpression().

lib/Target/X86/AsmParser/X86AsmParser.cpp

1371

Is it initialized properly?

1592

Info might be uninitialized here.

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1371

yes, but way down along the calling sequence (LookupInlineAsmIdentifier, lib/Sema/SemaStmtAsm,cpp)

1592

see previous note

[coby]

superseded by D37412

include/llvm/MC/MCParser/MCAsmParser.h

 class MCExpr;
 class MCInstPrinter;
 class MCInstrInfo;
 class MCStreamer;
 class MCTargetAsmParser;
 class SourceMgr;
 
 class InlineAsmIdentifierInfo {
+private:
+  enum IdDeclKind : uint8_t {
+    Default,
+    Variable,
+    ConstEnum,
+  };
+  IdDeclKind Kind;
+
 public:
   void *OpDecl;
-  bool IsVarDecl;
   unsigned Length, Size, Type;
-
+  APInt ConstIntValue;
+  bool isVarDecl() { return Kind == Variable; }
+  bool isConstEnum() { return Kind == ConstEnum; }
+  void setKindVariable() { Kind = Variable; }
+  void setKindConstEnum() { Kind = ConstEnum; }
   void clear() {
     OpDecl = nullptr;
-    IsVarDecl = false;
+    Kind = Default;
     Length = 1;
     Size = 0;
     Type = 0;
+    // On clear flush possibly old APInt value as a precaution;
+    ConstIntValue = APInt();
   }
 };
 
 /// \brief Generic Sema callback for assembly parser.
 class MCAsmParserSemaCallback {
 public:
   virtual ~MCAsmParserSemaCallback();
 
   virtual void *LookupInlineAsmIdentifier(StringRef &LineBuf,
                                           InlineAsmIdentifierInfo &Info,
                                           bool IsUnevaluatedContext) = 0;
   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.
 class MCAsmParser {
 public:
   using DirectiveHandler = bool (*)(MCAsmParserExtension*, StringRef, SMLoc);
   using ExtensionDirectiveHandler =
       std::pair<MCAsmParserExtension*, DirectiveHandler>;

lib/Target/X86/AsmParser/X86AsmParser.cpp

private:
   bool VerifyAndAdjustOperands(OperandVector &OrigOperands,
                                OperandVector &FinalOperands);
   std::unique_ptr<X86Operand> ParseOperand();
   std::unique_ptr<X86Operand> ParseATTOperand();
   std::unique_ptr<X86Operand> ParseIntelOperand();
   std::unique_ptr<X86Operand> ParseIntelOffsetOfOperator();
   bool ParseIntelDotOperator(const MCExpr *Disp, const MCExpr *&NewDisp);
   unsigned IdentifyIntelOperator(StringRef Name);
-  unsigned ParseIntelOperator(unsigned OpKind);
+  unsigned ParseIntelOperator(unsigned OpKind, bool AddImmPrefix);
   std::unique_ptr<X86Operand>
   ParseIntelSegmentOverride(unsigned SegReg, SMLoc Start, unsigned Size);
   std::unique_ptr<X86Operand> ParseRoundingModeOp(SMLoc Start, SMLoc End);
   bool ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine &SM);
   bool ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End);
   std::unique_ptr<X86Operand>
   ParseIntelBracExpression(unsigned SegReg, SMLoc Start, int64_t ImmDisp,
                            bool isSymbol, unsigned Size);
   bool ParseIntelIdentifier(const MCExpr *&Val, StringRef &Identifier,
                             InlineAsmIdentifierInfo &Info,
                             bool IsUnevaluatedOperand, SMLoc &End);
 
   std::unique_ptr<X86Operand> ParseMemOperand(unsigned SegReg, SMLoc StartLoc);
 

 }
 
 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,
     InlineAsmIdentifierInfo &Info, bool AllowBetterSizeMatch) {
   // 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 (isa<MCSymbolRefExpr>(Disp) && Info.OpDecl && !Info.isVarDecl()) {
     // 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
else if (Name.equals_lower("xor"))
     SM.onXor();
   else if (Name.equals_lower("and"))
     SM.onAnd();
   else
     return false;
   return true;
 }
 
 bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End) {
   MCAsmParser &Parser = getParser();
   const AsmToken &Tok = Parser.getTok();
 
   AsmToken::TokenKind PrevTK = AsmToken::Error;
   bool Done = false;
   while (!Done) {
     bool UpdateLocLex = true;
 
case AsmToken::Identifier: {
       } else if (!isParsingInlineAsm()) {
         if (getParser().parsePrimaryExpr(Val, End))
           return Error(Tok.getLoc(), "Unexpected identifier!");
         SM.onIdentifierExpr(Val, Identifier);
       } else if (unsigned OpKind = IdentifyIntelOperator(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 = ParseIntelOperator(OpKind);
+        int64_t Val = ParseIntelOperator(OpKind,SM.getAddImmPrefix());
         if (!Val)
           return true;
         StringRef ErrMsg;
         if (SM.onInteger(Val, ErrMsg))
           return Error(IdentLoc, ErrMsg);
       } else if (Identifier.find('.') != StringRef::npos &&
             PrevTK == AsmToken::RBrac) {
           return false;
       } else {
-        InlineAsmIdentifierInfo &Info = SM.getIdentifierInfo();
+        InlineAsmIdentifierInfo Info;
         if (ParseIntelIdentifier(Val, Identifier, Info,
                                  /*Unevaluated=*/false, End))
           return true;
+        // Check if the parsed identifier was a constant Integer. Here we
+        // assume Val is of type MCConstantExpr only when it is safe to replace
+        // the identifier with its constant value.
+        if (const MCConstantExpr *CE =
+                dyn_cast_or_null<const MCConstantExpr>(Val)) {
+          StringRef ErrMsg;
+          // Pass the enum identifier integer value to the SM calculator.
+          if (SM.onInteger(CE->getValue(), ErrMsg))
+            return Error(IdentLoc, ErrMsg);
+          // Match the behavior of integer tokens when getAddImmPrefix flag is
+          // set.
+          if (SM.getAddImmPrefix()) {
+            assert(isParsingInlineAsm() &&
+                   "Expected to be parsing inline assembly.");
+            // A single rewrite of the integer value is preformed for each enum
+            // identifier. This is only done when we are inside a bracketed
+            // expression.
+            size_t Len = End.getPointer() - IdentLoc.getPointer();
+            InstInfo->AsmRewrites->emplace_back(AOK_Imm, IdentLoc, Len,
+                                                CE->getValue());
+            break;
+          }
+        }
+        else {
+          // Notify the SM a variable identifier was found.
+          InlineAsmIdentifierInfo &SMInfo = SM.getIdentifierInfo();
+          SMInfo = Info;
           SM.onIdentifierExpr(Val, Identifier);
         }
+      }
       break;
     }
     case AsmToken::Integer: {
       StringRef ErrMsg;
       if (isParsingInlineAsm() && SM.getAddImmPrefix())
         InstInfo->AsmRewrites->emplace_back(AOK_ImmPrefix, Tok.getLoc());
       // Look for 'b' or 'f' following an Integer as a directional label
       SMLoc Loc = getTok().getLoc();
bool X86AsmParser::ParseIntelIdentifier(const MCExpr *&Val,
   } 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) &&
          "frontend claimed part of a token?");
 
+  // Check if the search yielded a constant integer (enum identifier).
+  if (Result && Info.isConstEnum()) {
+    // By creating MCConstantExpr we let the user of Val know it is safe
+    // to use as an explicit constant with value = ConstVal.
+    Val = MCConstantExpr::create(Info.ConstIntValue.getSExtValue(),
+                                 getParser().getContext());
+    return false;
+  }
   // If the identifier lookup was unsuccessful, assume that we are dealing with
   // a label.
   if (!Result) {
     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.

 }
 
 /// Parse the 'LENGTH', 'TYPE' and 'SIZE' operators.  The LENGTH operator
 /// returns the number of elements in an array.  It returns the value 1 for
 /// non-array variables.  The SIZE operator returns the size of a C or C++
 /// variable.  A variable's size is the product of its LENGTH and TYPE.  The
 /// TYPE operator returns the size of a C or C++ type or variable. If the
 /// variable is an array, TYPE returns the size of a single element.
-unsigned X86AsmParser::ParseIntelOperator(unsigned OpKind) {
+unsigned X86AsmParser::ParseIntelOperator(unsigned OpKind, bool AddImmPrefix) {
   MCAsmParser &Parser = getParser();
   const AsmToken &Tok = Parser.getTok();
   SMLoc TypeLoc = Tok.getLoc();
   Parser.Lex(); // Eat operator.
 
   const MCExpr *Val = nullptr;
   InlineAsmIdentifierInfo Info;
   SMLoc Start = Tok.getLoc(), End;
unsigned X86AsmParser::ParseIntelOperator(unsigned OpKind, bool AddImmPrefix) {
   
   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;
   }
   
-  // Rewrite the type operator and the C or C++ type or variable in terms of an
-  // immediate.  E.g. TYPE foo -> $$4
+  // Only when in bracketed mode, preform explicit rewrite. This is requierd to
+  // avoid rewrite collision.
+  if (AddImmPrefix) {
+    // Rewrite the type operator and the C or C++ type or variable in terms of
+    // an immediate. e.g. mov eax, [eax + SIZE _foo * $$4] ->
+    // mov eax, [eax + $$1 * $$4].
     unsigned Len = End.getPointer() - TypeLoc.getPointer();
     InstInfo->AsmRewrites->emplace_back(AOK_Imm, TypeLoc, Len, CVal);
+  }
   
   return CVal;
 }
 
 std::unique_ptr<X86Operand> X86AsmParser::ParseIntelOperand() {
   MCAsmParser &Parser = getParser();
   const AsmToken &Tok = Parser.getTok();
   SMLoc Start, End;
 
std::unique_ptr<X86Operand> X86AsmParser::ParseIntelOperand() {
   if (ParseIntelExpression(SM, End))
     return nullptr;
 
   bool isSymbol = SM.getSym() && SM.getSym()->getKind() != MCExpr::Constant;
   int64_t Imm = SM.getImm();
   if (SM.getSym() && SM.getSym()->getKind() == MCExpr::Constant)
     SM.getSym()->evaluateAsAbsolute(Imm);
 
-  if (StartTok.isNot(AsmToken::Identifier) &&
-      StartTok.isNot(AsmToken::String) && isParsingInlineAsm()) {
+  if (isParsingInlineAsm() && !isSymbol) {
     unsigned Len = Tok.getLoc().getPointer() - Start.getPointer();
-    if (StartTok.getString().size() == Len)
-      // Just add a prefix if this wasn't a complex immediate expression.
-      InstInfo->AsmRewrites->emplace_back(AOK_ImmPrefix, Start);
-    else
-      // Otherwise, rewrite the complex expression as a single immediate.
       InstInfo->AsmRewrites->emplace_back(AOK_Imm, Start, Len, Imm);
   }
 
   if (getLexer().isNot(AsmToken::LBrac)) {
     // If a directional label (ie. 1f or 2b) was parsed above from
     // ParseIntelExpression() then SM.getSym() was set to a pointer to
     // to the MCExpr with the directional local symbol and this is a
     // memory operand not an immediate operand.
     if (isSymbol) {