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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers 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");
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97568