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

[coby via Phabricator via llvm-commits]

coby added inline comments.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1310
 }
-
-bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End) {
+bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End,
+                                        bool &ReplaceEnumIdentifier) {
----------------
blank line omitted


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1378
+        // 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.
----------------
assumption ~~> assertion


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1380
+        // the identifier with its constant value.
+        if (const MCConstantExpr *CE =
+            dyn_cast_or_null<const MCConstantExpr>(Val)) {
----------------
I think this whole section better suites within its own function. something like 'ParseInlineAsmEnumValue'


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1383
+          StringRef ErrMsg;
+          // SM should treat the value as it would an explicit integer in the 
+          // expression.
----------------
rephrase


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1385
+          // expression.
+          if(SM.onInteger(CE->getValue(), ErrMsg)) 
+            return Error(IdentLoc, ErrMsg);
----------------
clang format


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1388
+          // 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
----------------
'isParsingInlineAsm()' is unnecessary here (you can only reach this piece of code when parsing inline asm)


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1826
   }
-
-  // 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);
-
+  // Only when in bracketed mode, preform explicit rewrite
+  if (AddImmPrefix) {
----------------
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.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1907
     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.
+    if (StartTok.getString().size() != Len || ReplaceEnumIdentifier)
+      // Rewrite the complex expression as a single immediate.
----------------
you may just perform an AOK_Imm rewrite regardless the complexity of the immediate expression, and neglect 'ReplaceEnumIdentifier'


Repository:
  rL LLVM

https://reviews.llvm.org/D33278