[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:1488
+                                    AsmToken::TokenKind::Star))
+          return Error(IdentLoc, ErrMsg);
       }
----------------
RKSimon wrote:
> clang-format
I'm using clang-format for every new patch prepared for phab and this style was generated by clang-format (I've just checked it again).


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1514
+                                      AsmToken::TokenKind::Star))
+            return Error(Loc, ErrMsg);
           End = consumeToken();
----------------
RKSimon wrote:
> clang-format
See above.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1596
+    Disp = Imm; // It can be an immediate displacement only (maybe 0).
   }
 
----------------
RKSimon wrote:
> What is this accomplishing? Pulling on ImmVal makes sense, but the rest looks wrong - for instance, if Disp is a MCExpr::Constant doesn't that constant value is overriden by Imm instead of being added together?
Oh, yes: if Disp is Constant and Imm != 0 then we should add 2 values and it was already done in one of the previous versions but disappeared. Tnank you. I'll add this clause asap. But the current override of Disp with Imm is correct as well because we have Disp == SM.getSym(). On the other hand the condition "if (!Disp || (Disp->getKind() == llvm::MCExpr::Constant && ImmVal))" looks wrong.


https://reviews.llvm.org/D35621