[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,
----------------
(#)
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?


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:601
+      case IES_MULTIPLY:
+        if (PrevState == IES_REGISTER &&
+            SymRef->getKind() == llvm::MCExpr::Constant) {
----------------
if comment (#) evaluate to false (i.e. MSVC inline-assembler does not support Assembly Constants, and we do not plan to do it either) than you should error here appropriately (i.e. "symbolic scale aren't supported on inline-asm)


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:603
+            SymRef->getKind() == llvm::MCExpr::Constant) {
+          // IndexRegister + SymbolicScale * Register
+          auto *Constant = cast<MCConstantExpr>(SymRef);
----------------
BaseReg + SymScale * IndexReg


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:614
       case IES_MINUS:
       case IES_NOT:
         State = IES_INTEGER;
----------------
What about other operations?
If we are to deal with a symbolic constant as a constant than it should be able to participate in whatever operation an integer is allowed


================
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;
----------------
Note that this patch breaks //'ms-inline-asm.c'// on //'clang/test/Sema'//


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1015
 
+  // In MS inline-asm we allow variables to be named as registers, and
+  // give them precedence over actual registers
----------------
Rebase
This is a part of an old code of mine which got reverted


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1463
           return Error(Tok.getLoc(), "Unexpected identifier!");
-        SM.onIdentifierExpr(Val, Identifier);
+        bool isNextMult =
+            getLexer().getTok().getKind() == AsmToken::TokenKind::Star;
----------------
I suggest we'll adopt a more general approach and deal with assembly constants as a whole, and not just this private case. wouldn't it simplify the work?


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1581
+  int64_t ImmVal = SM.getImm();
   if (const MCExpr *Sym = SM.getSym()) {
     // A symbolic displacement.
----------------
```
if (Disp = SM.getSym() && isParsingInlineAsm())
```



https://reviews.llvm.org/D35621