[PATCH] D97501: [ARM] support symbolic expressions as branch target

Fri Feb 26 11:56:07 PST 2021

nickdesaulniers added inline comments.

================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:1079
     }
-    return false;
+    // Delay the checks of symbolic values until they are resolved
+    return true;
----------------
jcai19 wrote:
> nickdesaulniers wrote:
> > jcai19 wrote:
> > > nickdesaulniers wrote:
> > > > I cant help but wonder whether we should leave `isSignedOffset` as is, and modify decisions to check vs delay up into the relevant call site?  All of these `is<Something>` checks for operands have nice symmetry, so it's curious to disturb it just for `isSignedOffset`.
> > > That's what I first implemented: I modified the call site to not report errors if the target of a b.w instruction is not a constant expression. But looking at the code again, I realized (A) isSignedOffset is local to this file and most of its uses are for checking the range of different branch instructions, so hopefully modifying this function won't have too big an impact on other instructions, and (B) more importantly if it's safe to delay the check of single symbolic values (which is essentially a special case of symbolic expressions ), I don't see why we should not hold off the checks for more general symbolic expressions. This should also allow symbolic expressions in the other branch instructions that call this function. WDYT?
> > So `isSignedOffset` is attempting down casts of `MCExpr` to figure out whether the operand is a signed offset.
> > 
> > For the case we care about, looking at the various derived classes of `MCExpr` in llvm/include/llvm/MC/MCExpr.h, it looks like the case we care about is a `MCBinaryExpression`.
> > 
> > That case is not checked in `isSignedOffset`.  I'm not sure that it should be, either, since the answer is "maybe, but I won't know until after layout."
> > 
> > The relevant call site is `ARMAsmParser::validateInstruction`, in the `case` for `ARM::t2B`.  There's a comment above the case `"Final range checking for Thumb unconditional branch instructions."`.  Your XFAIL test case added to llvm/test/MC/ARM/thumb2-branch-ranges.s demonstrates that such range check is _not_ final.  Oh, the error is different and less developer friendly; the relocation itself fails to be encoded.
> > 
> > So I think it would be better to amend the call site to check whether `isa<MCBinaryExpr>(static_cast<ARMOperand &>(*Operands[op]).getImm())`, and skip the check.  (It does seem that many other cases probably suffer something similar; different instructions eagerly assume they have `MCConstantExpr` operands and probably don't work when expressions are used as operands in assembler.  But perhaps we could fix this up first for `t2B` or see if we can get it working, then clean up the rest.)
> > 
> > Then I'm curious whether we can call `ARMAsmParser::validateInstruction` for each instruction that has operands that we resolve post fragment layout?  Then we'd have the nicer existing error message that `"branch target out of range"`?  If not, I guess that's ok, since the relocation won't be encodeable, so garbage input will still fail to assemble, and at least we'll support asm expressions as operands.
> > So `isSignedOffset` is attempting down casts of `MCExpr` to figure out whether the operand is a signed offset.
> > 
> > For the case we care about, looking at the various derived classes of `MCExpr` in llvm/include/llvm/MC/MCExpr.h, it looks like the case we care about is a `MCBinaryExpression`.
> 
> I think MCBinaryExpression will cover cases like b.w . + (2f - 1b + 2) but not if we remove the parentheses.  
> 
> > 
> > That case is not checked in `isSignedOffset`.  I'm not sure that it should be, either, since the answer is "maybe, but I won't know until after layout."
> > 
> > The relevant call site is `ARMAsmParser::validateInstruction`, in the `case` for `ARM::t2B`.  There's a comment above the case `"Final range checking for Thumb unconditional branch instructions."`.  Your XFAIL test case added to llvm/test/MC/ARM/thumb2-branch-ranges.s demonstrates that such range check is _not_ final.  Oh, the error is different and less developer friendly; the relocation itself fails to be encoded.
> > 
> > So I think it would be better to amend the call site to check whether `isa<MCBinaryExpr>(static_cast<ARMOperand &>(*Operands[op]).getImm())`, and skip the check.  (It does seem that many other cases probably suffer something similar; different instructions eagerly assume they have `MCConstantExpr` operands and probably don't work when expressions are used as operands in assembler.  But perhaps we could fix this up first for `t2B` or see if we can get it working, then clean up the rest.)
> > 
> > Then I'm curious whether we can call `ARMAsmParser::validateInstruction` for each instruction that has operands that we resolve post fragment layout?  Then we'd have the nicer existing error message that `"branch target out of range"`?  If not, I guess that's ok, since the relocation won't be encodeable, so garbage input will still fail to assemble, and at least we'll support asm expressions as operands.
> 
> I have created https://reviews.llvm.org/D97568 like you suggested, which was also what I first implemented. I think the code is a bit redundant because we need to check if an expression is MCConstant expression twice, but maybe like you said, we could first fix one case and clean the rest up up bit by bit.
> 
> 
> I think MCBinaryExpression will cover cases like b.w . + (2f - 1b + 2) but not if we remove the parentheses.

Which derived class is it without parens in the expression? Maybe we should be checking for either?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97501