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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
