[PATCH] D35621: X86 Asm can't work properly with symbolic Scale

[Andrew V. Tischenko via Phabricator via llvm-commits]

avt77 added inline comments.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:591
+    }
+    bool onIdentifierExpr(const MCExpr *SymRef, StringRef SymRefName,
+                          StringRef &ErrMsg, bool isParsingInlineAsm,
----------------
coby wrote:
> (#)
> Note that the two conditions (isParsingInlineAsm) and (SymRef->getKind() == llvm::MCExpr::Constant) never coexist in the current code due to the way Assembly Constants are being treated when parsing MS InlineAsm. 
> I'm not sure whether it is intended or not - does MSVC inline assembler allow any kind of Assembly Constants?
Do you mean that this function can't be called if isParsingInlineAsm == true?


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:616
         State = IES_INTEGER;
-        Sym = SymRef;
-        SymName = SymRefName;
-        IC.pushOperand(IC_IMM);
+        if (isParsingInlineAsm || !isNextMult) {
+          Sym = SymRef;
----------------
coby wrote:
> Note that this patch breaks //'ms-inline-asm.c'// on //'clang/test/Sema'//
I'm no sure I understand you here. As result of the patch we have the following:

 void t3() {
--  __asm { mov eax, [eax] UndeclaredId } // expected-error {{unknown token in expression}} expected-error {{use of undeclared label 'UndeclaredId'}}
+  __asm { mov eax, [eax] UndeclaredId } // expected-error {{invalid address operation}} expected-error {{use of undeclared label 'UndeclaredId'}}

   // FIXME: Only emit one diagnostic here.
   // expected-error at +3 {{use of undeclared label 'A'}}
   // expected-error at +2 {{unexpected type name 'A': expected expression}}
--  // expected-error at +1 {{unknown token in expression}}
+  // expected-error at +1 {{invalid address operation}}
   __asm { mov eax, [eax] A }
 }

As you see "unknown token" message was replaced by "invalid address operation". Does it mean "break" for this test?


https://reviews.llvm.org/D35621