Page MenuHomePhabricator

[ARM] PR25722 Support and tests for transform of LDR rt, = to MOV
ClosedPublic

Authored by peter.smith on May 11 2016, 2:57 AM.

Details

Summary

This change implements the transformation in processInstruction() for the LDR rt, =expression to MOV rt, expression when the expression can be evaluated and can fit into the immediate field of the MOV or a MVN.

Across the ARM and Thumb instruction sets there are several cases to consider, each with a different range of representatble constants.
In ARM we have:

  • Modified immediate (All ARM architectures)
  • MOVW (v6t2 and above)

In Thumb we have:

  • Modified immediate (v6t2, v7m and v8m.mainline)
  • MOVW (v6t2, v7m, v8.mainline and v8m.baseline)
  • Narrow Thumb MOV that can be used in an IT block (non flag-setting)

If the immediate fits any of the available alternatives then we make the transformation.

Fixes 25722.

Diff Detail

Event Timeline

peter.smith retitled this revision from to [ARM] PR25722 Support and tests for transform of LDR rt, = to MOV.
peter.smith updated this object.
peter.smith added a subscriber: llvm-commits.
rengolin updated this object.May 11 2016, 3:00 AM

Couple of comments on this one, but only one that looks like it could be a real problem:

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
6907

ARM::MOVi16's actual predicate is "[IsARM, HasV6T2]". It's probably an academic distinction, but checking for v8m in ARM-mode code looks weird.

6934

It looks like SP is a permitted register for t2LDRpci but not the Thumb MOVs.

Hello Tim,

Thanks very much for the review. I'll send an update out tomorrow.

Peter

peter.smith edited edge metadata.

I've updated the patch to not do any kind of translation when the destination is SP or PC. Now that I think about it I have seen bootcode where the stack pointer has been set using ldr sp,=value so this would have caused problems.

I've added changed the ARM check from hasV8MBaseline to has v6T2Ops as that matches TableGen and befits the ARM presence of MOVW in V6T2 onwards.

t.p.northover accepted this revision.May 12 2016, 9:36 AM
t.p.northover edited edge metadata.

Thanks for updating the patch Peter. This looks good to me now.

Now that I think about it I have seen bootcode where the stack pointer has been set using ldr sp,=value so this would have caused problems.

That doesn't surprise me at all, though I couldn't actually remember seeing any.

Cheers.

Tim.

This revision is now accepted and ready to land.May 12 2016, 9:36 AM
rengolin closed this revision.May 12 2016, 2:39 PM
rengolin edited edge metadata.

Committed in r269354