[PATCH] D98916: [ARM] support symbolic expression as immediate in memory instructions

[Jian Cai via Phabricator via llvm-commits]

jcai19 added inline comments.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:1099-1117
   bool isThumbMemPC() const {
     int64_t Val = 0;
     if (isImm()) {
       if (isa<MCSymbolRefExpr>(Imm.Val)) return true;
       const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(Imm.Val);
       if (!CE) return false;
       Val = CE->getValue();
----------------
nickdesaulniers wrote:
> Something still really bugs me about these `is*` functions; perhaps I'm still hung up on D97568.
> 
> I understand that we can't validate immediates when expressions are used until post fixup; doing such validation of operands before fixups are performs seems like a major phase ordering issue in LLVM's assemblers.
> 
> So for a lot of these `is*` functions, if we don't have an immediate operand, we can't tell if it fits whatever constraints we're checking.  This change makes these functions return `true` as in "maybe."  Rather than return a type that's a tri-state for "definitely yes," "definitely no," or "can't tell until post fixups."  Then other `is*` functions don't have that added logic, like `isFPImm` below, `isLEOffset` and `isSignedOffset` above, etc.  I'm very concerned about `isARMBranchTarget`/`isThumbBranchTarget`.
> 
> I almost wonder if fixing the phase ordering in LLVM will fix this more generally for all instructions/operands or even additional ISAs.  Perhaps let's hold onto this change, but see how feasible it would be to validate instructions post fixups in LLVM?  Then we can keep these `is` functions more strict about only accepting immediate operands.
> I almost wonder if fixing the phase ordering in LLVM will fix this more generally for all instructions/operands or even additional ISAs.  Perhaps let's hold onto this change, but see how feasible it would be to validate instructions post fixups in LLVM?  Then we can keep these `is` functions more strict about only accepting immediate operands.

I realized that resolving some of these fixups also depends on the chose encoding of the instructions between the symbols, i.e. whether it is 16 or 32 bits. So it may be difficult to switch the order of the two phases. Maybe we can add different an option to allow users to decide if size is a major concern, which would allow us to choose a better encoding. It's suboptimal but I suspect such code is not dominant in most code bases, so maybe it's acceptable. WDYT?



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98916/new/

https://reviews.llvm.org/D98916