Page MenuHomePhabricator

[ARM] Do not emit ldrexd/strexd on Cortex-M chips
ClosedPublic

Authored by aykevl on Feb 2 2021, 12:14 PM.

Details

Summary

The ldrexd/strexd instructions are not supported on M-class chips, see for example https://developer.arm.com/documentation/dui0489/e/arm-and-thumb-instructions/memory-access-instructions/ldrex-and-strex which says:

All these 32-bit Thumb instructions are available in ARMv6T2 and above, except that LDREXD and STREXD are not available in the ARMv7-M architecture.

Looking at the ARMv8-M architecture, it appears that these instructions aren't supported either. The Architecture Reference Manual lists ldrex/strex but not ldrexd/strexd: https://developer.arm.com/documentation/ddi0553/bn/

Compiler Explorer example on LLVM 11.0.0, which incorrectly emits ldrexd/strexd instructions: https://llvm.godbolt.org/z/5qqPnE

Diff Detail

Event Timeline

aykevl created this revision.Feb 2 2021, 12:14 PM
aykevl requested review of this revision.Feb 2 2021, 12:14 PM
aykevl edited the summary of this revision. (Show Details)Feb 2 2021, 12:17 PM
aykevl edited the summary of this revision. (Show Details)
SjoerdMeijer added inline comments.Feb 3 2021, 6:07 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
18761

I think this could benefit from a comment saying that this is a special case for LDREXD and STREXD as they are not available in v7-m and v8-m.

llvm/test/CodeGen/ARM/atomic-64bit.ll
5

Can you add an armv8m RUN line too?

aykevl updated this revision to Diff 321151.Feb 3 2021, 10:30 AM
  • added comment to shouldExpandAtomicCmpXchgInIR explaining Cortex-M exception
  • added checks for armv8m architecture (which are the same as armv7m)

Thanks for the review! I've updated the patch accordingly.

llvm/lib/Target/ARM/ARMISelLowering.cpp
18761

I've added a comment. The situation is really the same as for shouldExpandAtomicRMWInIR etc.

llvm/test/CodeGen/ARM/atomic-64bit.ll
5

Sure, done. As far as I can see, it's the same for armv7m as for armv8m: they both only support 32-bit (4 byte) atomic operations. Not entirely sure about the check prefix but it's for the Cortex-M profile and others are similarly short.

SjoerdMeijer accepted this revision.Feb 4 2021, 2:16 AM

Cheers, LGTM.

This revision is now accepted and ready to land.Feb 4 2021, 2:16 AM
This revision was landed with ongoing or failed builds.Feb 4 2021, 12:56 PM
This revision was automatically updated to reflect the committed changes.

@SjoerdMeijer thank you for the quick review!