[PATCH] D97568: [ARM] support symbolic expressions as branch target in b.w

[Jian Cai via Phabricator via llvm-commits]

jcai19 added inline comments.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:7967
     int Op = (Operands[2]->isImm()) ? 2 : 3;
     if (!static_cast<ARMOperand &>(*Operands[Op]).isSignedOffset<20, 1>())
       return Error(Operands[Op]->getStartLoc(), "branch target out of range");
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > It's definitely not great seeing this problem across multiple different instructions; in particular I suspect that `t2Bcc` is the same operation as `t2B` (branch), just with a condition code (cc) and smaller immediate.
> > 
> > I still think the callers should defer checks for `MCBinaryExpr` (as this patch does), but then seeing things like `isThumbBranchTarget`/`isARMBranchTarget` makes me nervous that we can't validate `MCBinaryExpr` operands until after adjusting fixups (very late) for many other instructions.
> > 
> > If `MCSymbolRefExpr` is just the `.` operator in assembler, and `MCBinaryExpr` is for expressions, I wonder if `isUnsignedOffset`, `isSignedOffset`, `isLEOffset`, and `isThumbMemPC` should all return `true` for `MCBinaryExpr`?
> > 
> > `isARMBranchTarget`/`isThumbBranchTarget` default to `true` (for example for `MCSymbolRefExpr` and `MCBinaryExpr`, but the list above do not.  And that's curious to me.  We can't know if these immediates satisfy some range constraints until after fixup; right now these methods are a mix of what they permit when expressions involving `.` are used.
> And part of that is that these `isSignedOffset` and friends all return `bool`, when for `MCSymbolRefExpr` and `MCBinaryExpr` operands the answer has a third possible state which is "maybe, but I won't know until adjusting fixups" which is one of the last things the assembler does, without rerunning instruction validation.  isUnsignedOffset, isSignedOffset, isLEOffset, and isThumbMemPC all respond "no" conservatively; isARMBranchTarget/isThumbBranchTarget respond "sure" perhaps too eagerly.  I suspect given a `MCSymbolRefExpr` or `MCBinaryExpr`, isARMBranchTarget/isThumbBranchTarget both respond true, when after fixups the answer perhaps should have been "no."
Agreed, I will take a look at the code and see if we could refactor it for a more permanent solution.  Also `.` gets converted into a symbol like any regular label, so MCSymbolRefExpr covers any expressions with a single label or `.`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97568