[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:
> > 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
> In this case I could simply produce error message if (isParsingInlineAsm() && SymRef->getKind() == llvm::MCExpr::Constant) , right?
Currently, yes. But I frankly believe we should completely ignore InlineAsm (i.e. confine the patch to assembly level only, as proposed on line 1461), or conduct a better research regarding the way assembly constants are represented (likely - aren't) by msvc inline-assembler, and act upon the findings


https://reviews.llvm.org/D35621