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

[Jian Cai via Phabricator via llvm-commits]

jcai19 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;
----------------
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?


================
Comment at: llvm/test/MC/ARM/thumb2-branch-ranges.s:100-101
+// branch to thumb function resolved at assembly time
+// CHECK-NOT: error
+// CHECK: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
+        b.w start8 - start7 + 0x1000000
----------------
nickdesaulniers wrote:
> This checks that `error` is not printed in the output, but then checks that `error: ...` is? Or am I misunderstanding that? (I guess I don't understand the point of the `CHECK-NOT`).
Yes I have the same question. I copied this line from previous tests, but maybe I could remove this line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97501