[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

68

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

68

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

40

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.

68

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.

1382

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

1382

not addressed?

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1310–1312

blank line omitted

1379

assumption ~~> assertion

1381

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

1384

rephrase

1386

clang format

1389

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

1832

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.

1913

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

1379

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.

1381

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

1382

Persistent text wrapping of my editor, fixed.

1384

Your comment is too general, I made some clarifications.

1389

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

1832

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.

1913

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

1400–1401

clang-format this

1913

Nice. :)

[chapuni]

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

lib/Target/X86/AsmParser/X86AsmParser.cpp

1373–1374

Is it initialized properly?

1596

Info might be uninitialized here.

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1373–1374

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

1596

see previous note

[coby]

superseded by D37412

include/llvm/MC/MCParser/MCAsmParser.h

 class MCInstrInfo;
 class MCStreamer;
 class MCTargetAsmParser;
 class SourceMgr;
 
 class InlineAsmIdentifierInfo {
 public:
   void *OpDecl;
   bool IsVarDecl;
   unsigned Length, Size, Type;
-
+  bool IsConstEnum;
+  APInt ConstIntValue;
   void clear() {
     OpDecl = nullptr;
     IsVarDecl = false;
     Length = 1;
     Size = 0;
     Type = 0;
+    IsConstEnum = false;
+    // 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:
   std::unique_ptr<X86Operand> ParseIntelOffsetOfOperator();
   bool ParseIntelDotOperator(const MCExpr *Disp, const MCExpr *&NewDisp);
   unsigned IdentifyIntelOperator(StringRef Name);
   unsigned ParseIntelOperator(unsigned OpKind);
   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);
+  bool ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End, 
+                            bool &ReplaceEnumIdentifier);
   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);
bool X86AsmParser::ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine &SM) {
   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) {
+bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End, 
+                                        bool &ReplaceEnumIdentifier) {
+  ReplaceEnumIdentifier = false;
   MCAsmParser &Parser = getParser();
   const AsmToken &Tok = Parser.getTok();
 
   AsmToken::TokenKind PrevTK = AsmToken::Error;
   bool Done = false;
   while (!Done) {
     bool UpdateLocLex = true;
 
case AsmToken::Identifier: {
           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;
+        Info.clear();
         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;
+          // SM should treat the value as it would an explicit integer in the 
+          // expression.
+          if(SM.onInteger(CE->getValue(), ErrMsg)) 
+            return Error(IdentLoc, ErrMsg);
+          // In case we are called on a bracketed expression,
+          if (isParsingInlineAsm() && SM.getAddImmPrefix()) {
+            // A single rewrite of the integer value is preformed for each enum
+            // identifier. This is only done when we are inside a bracketed 
+            // expression in order to match the behavior for the equivalent 
+            // integer tokens.
+            size_t Len = End.getPointer() - IdentLoc.getPointer();
+            InstInfo->AsmRewrites->emplace_back(AOK_Imm, IdentLoc,Len, 
+                                                CE->getValue());
+            break;
+          }
+          // Set force rewrite flag only when not bracketed expression.
+          ReplaceEnumIdentifier = true;
+        } 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();
if (getLexer().isNot(AsmToken::LBrac))
     return ErrorOperand(BracLoc, "Expected '[' token!");
   Parser.Lex(); // Eat '['
 
   SMLoc StartInBrac = Parser.getTok().getLoc();
   // Parse [ Symbol + ImmDisp ] and [ BaseReg + Scale*IndexReg + ImmDisp ].  We
   // may have already parsed an immediate displacement before the bracketed
   // expression.
   IntelExprStateMachine SM(ImmDisp, /*StopOnLBrac=*/false, /*AddImmPrefix=*/true);
-  if (ParseIntelExpression(SM, End))
+  bool ReplaceEnumIdentifier;
+  if (ParseIntelExpression(SM, End, ReplaceEnumIdentifier))
     return nullptr;
 
   const MCExpr *Disp = nullptr;
   if (const MCExpr *Sym = SM.getSym()) {
     // A symbolic displacement.
     Disp = Sym;
     if (isParsingInlineAsm())
       RewriteIntelBracExpression(*InstInfo->AsmRewrites, SM.getSymName(),
do {
     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) &&
          "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.
unsigned X86AsmParser::ParseIntelOperator(unsigned OpKind) {
   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
   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() {
   // Parse [ BaseReg + Scale*IndexReg + Disp ].
   if (getLexer().is(AsmToken::LBrac))
     return ParseIntelBracExpression(/*SegReg=*/0, Start, /*ImmDisp=*/0, false,
                                     Size);
 
   AsmToken StartTok = Tok;
   IntelExprStateMachine SM(/*Imm=*/0, /*StopOnLBrac=*/true,
                            /*AddImmPrefix=*/false);
-  if (ParseIntelExpression(SM, End))
+  // The parsed expression may contain enum identifier tokens which we must 
+  // replace, ReplaceEnumIdentifier flag lets us know when to force rewrite.
+  bool ReplaceEnumIdentifier;
+  if (ParseIntelExpression(SM, End, ReplaceEnumIdentifier))
     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) &&
+  if ((ReplaceEnumIdentifier ||
+      StartTok.isNot(AsmToken::Identifier)) &&
       StartTok.isNot(AsmToken::String) && isParsingInlineAsm()) {
     unsigned Len = Tok.getLoc().getPointer() - Start.getPointer();
-    if (StartTok.getString().size() == Len)
+    if (StartTok.getString().size() == Len && !ReplaceEnumIdentifier)
       // 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.