[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:1596
+    Disp = Imm; // It can be an immediate displacement only (maybe 0).
   }
 
----------------
avt77 wrote:
> 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.
Oh and Oh: I recalled the situation:
  if (Disp is Constant) it means Disp == SM.getSym() (see assertion): it's the same value that's why we don't replace one with other. I could simplify it like here:

  else if (!Disp)
    Disp = Imm; // It can be an immediate displacement only (maybe 0).
 
OK? 
One more time: if (SM.getSym() != nullptr) and it's a constant then SM.getImm() returns the value of the constant.


https://reviews.llvm.org/D35621