[PATCH] D149443: [ARM] add Thumb-1 8-bit movs/adds relocations to LLVM

Thu Jun 15 09:28:27 PDT 2023

stuij added a comment.

In D149443#4403186 <https://reviews.llvm.org/D149443#4403186>, @john.brawn wrote:

> A few comments:
>
> - Please run clang-format on the patch (pre-merge is showing as failed due to this)
> - We should have a test to check that we're emitting the right relocation (probably something similar to test/MC/ARM/thumb-movwt-reloc.s)
> - There should be something in ARMAsmParser::validateInstruction to check that the expression is valid, like we do for movw/movt, as currently you can do `movs r0, :lower16:some_symbol` and it's accepted and results in a 16-bit mov with a movw relocation.
> - Something weird is happening when I try to use these expressions in movs on v7m. If I try to assemble `movs r0, :upper8_15:some_symbol` I get "error: expected relocatable expression", and looking at the `llvm-mc --show-encoding --show-inst-operands` it's selected movs.w instead of 16-bit mov for some reason.

the recent changes should have addressed all of these

================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:498-505
+  case ARM::fixup_arm_thumb_upper_8_15:
+    return (Value & 0xff000000) >> 24;
+  case ARM::fixup_arm_thumb_upper_0_7:
+    return (Value & 0x00ff0000) >> 16;
+  case ARM::fixup_arm_thumb_lower_8_15:
+    return (Value & 0x0000ff00) >> 8;
+  case ARM::fixup_arm_thumb_lower_0_7:
----------------
john.brawn wrote:
> The calculation here isn't correct when IsResolved is false. In that case we're calculating the addend for a relocation, and the value of that will be `Value&0xff` for all four of these fixups (movw/movt has similar behaviour). 
done

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149443