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

Andrew V. Tischenko via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 4 03:13:21 PDT 2017


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





More information about the llvm-commits mailing list