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

[Matan via Phabricator via llvm-commits]

mharoush marked 4 inline comments as done.
mharoush added inline comments.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1382
+        if (const MCConstantExpr *CE = 
+            dyn_cast_or_null<const MCConstantExpr>(Val)) {
+          StringRef ErrMsg;
----------------
rnk wrote:
> rnk wrote:
> > Please use clang-format here and elsewhere
> not addressed?
Persistent text wrapping of my editor, fixed.


================
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.
----------------
coby wrote:
> assumption ~~> assertion
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.


================
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)) {
----------------
coby wrote:
> I think this whole section better suites within its own function. something like 'ParseInlineAsmEnumValue'
not much added value here and only a few lines can be moved (~5). Its not really worth it.


================
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.
----------------
coby wrote:
> rephrase
Your comment is too general, I made some clarifications.


================
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
----------------
coby wrote:
> 'isParsingInlineAsm()' is unnecessary here (you can only reach this piece of code when parsing inline asm)
This is just for readability and consistency with SM.onInteger(). Replaced with assertion to ease your mind


================
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) {
----------------
coby wrote:
> 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.
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.


================
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.
----------------
coby wrote:
> you may just perform an AOK_Imm rewrite regardless the complexity of the immediate expression, and neglect 'ReplaceEnumIdentifier'
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D33278