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

[coby via Phabricator via llvm-commits]

coby added inline comments.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:591
+    }
+    bool onIdentifierExpr(const MCExpr *SymRef, StringRef SymRefName,
+                          StringRef &ErrMsg, bool isParsingInlineAsm,
----------------
avt77 wrote:
> 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?
I mean that (isParsingInlineAsm() && SymRef->getKind() == llvm::MCExpr::Constant) will never yield 'true' on current code state.
IMHO, we should aim for assembly level only.
So, combining the two we may limit this patch to Assembly parsing only


================
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;
----------------
avt77 wrote:
> avt77 wrote:
> > 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?
> Don't know the reason but "bullet" above means "minus".
As long as the test remains unchanged :)
But again, I don't think that MS InlineAsm should be affected by this change.


https://reviews.llvm.org/D35621