[PATCH] D69411: [MC] Calculate difference of symbols in two fragments when possible

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Thanks for the patch!  Looks like this was much simpler to support than I expected.



================
Comment at: llvm/lib/MC/MCExpr.cpp:521
+  const MCFragment *FragA = SA.getFragment();
+  const MCFragment *FragB = SB.getFragment();
 
----------------
Move these closer to their use below. It is nice to have them all declared together, but it would be nicer to bail as early as possible without doing more work than necessary.


================
Comment at: llvm/lib/MC/MCExpr.cpp:556
 
-  const MCSection &SecA = *SA.getFragment()->getParent();
-  const MCSection &SecB = *SB.getFragment()->getParent();
+     // Pointers to Thumb symbols need to have their low-bit set to allow
+   if ((&SecA != &SecB) && !Addrs)
----------------
What's going on with this comment?  Looks like it was copied, not moved to L566?


================
Comment at: llvm/lib/MC/MCExpr.cpp:561
+   if (SecB.getFragmentList().getNextNode(*FragB) == FragA &&
+       dyn_cast<MCDataFragment>(FragA) && dyn_cast<MCDataFragment>(FragB)) {
+     Addend +=
----------------
if you're throwing away the result of a `dyn_cast`, prefer `isa`.
http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

Or, if we're checking `dyn_cast` and reusing via `cast`, consider just saving the result of the `dyn_cast` to a local variable and discarding `cast`.


================
Comment at: llvm/lib/MC/MCExpr.cpp:568
+    // for interworking.
+    if (Asm->isThumbFunc(&SA))
+      Addend |= 1;
----------------
This patch adds checks for thumb, but the test case doesn't use a thumb target triple. Consider adding tests that exercise the newly added code.


================
Comment at: llvm/lib/MC/MCExpr.cpp:573
+    // correct offset in .gcc_except_table
+    if (Asm->getBackend().isMicroMips(&SA))
+      Addend |= 1;
----------------
MicroMips?


================
Comment at: llvm/lib/MC/MCExpr.cpp:580
+    return;
+  }
+
----------------
It looks like this block was copied from L529-L548. Is it possible to adjust the condition on L529 and addend calculation on L532 to support this case, rather than duplicating so much code? Looks like this repeats again below. Is it possible to refactor some of this, maybe into its own method, for better code reuse?


================
Comment at: llvm/test/MC/AsmParser/directive_if_with_dot_symbol.s:1
+@ RUN: llvm-mc -filetype=obj -triple armv7a-linux-gnueabihf %s -o -
+
----------------
Is it possible to add tests for other ISA's, too?


================
Comment at: llvm/test/MC/AsmParser/directive_if_with_dot_symbol.s:4
+nop
+.arch_extension sec
+9997: nop ; .if . - 9997b == 2 ; nop ; .endif ;
----------------
is this ISA extension needed to test this feature?


================
Comment at: llvm/test/MC/AsmParser/directive_if_with_dot_symbol.s:6
+9997: nop ; .if . - 9997b == 2 ; nop ; .endif ;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: error: expected absolute expression
----------------
We're checking that an error occurs? Wouldn't a test for this new feature test that an error does not occur? Or am I misunderstanding the test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69411