This is an archive of the discontinued LLVM Phabricator instance.

[Cortex-M0] Atomic lowering
ClosedPublic

Authored by weimingz on Oct 29 2016, 12:26 PM.

Details

Summary

ARMv6m supports dmb etc fench instructions but not ldrex/strex etc. So for some atomic load/store, LLVM should inline instructions instead of lowering to __sync_ calls.

Diff Detail

Event Timeline

weimingz updated this revision to Diff 76315.Oct 29 2016, 12:26 PM
weimingz retitled this revision from to [Cortex-M0] Atomic lowering.
weimingz updated this object.
weimingz set the repository for this revision to rL LLVM.
weimingz added a subscriber: llvm-commits.
efriedma requested changes to this revision.Oct 31 2016, 1:28 PM
efriedma added a reviewer: efriedma.
efriedma added a subscriber: efriedma.

Explicitly checking isARMv6m() is wrong... we want to check features, not specific architectures. I think here, we're interested here in the behavior of targets with the following characteristics:

(1) Are in thumb mode (isThumb())
(2) Support a memory barrier instruction (hasAnyDataBarrier())
(3) Don't support ldrex/strex (!hasV8MBaselineOps())

I think ARMv6sm also has these characteristics; checking features makes the code more clear, and avoids mistakes.


It would also be a good idea to add tests to check that this patch doesn't change the generated code for other atomic operations.

This revision now requires changes to proceed.Oct 31 2016, 1:28 PM

Hi Eli,

I think the problem is the feature for armv6m is not well defined.
ARMv6m is mostly Thumb1, but it also supports 5 32-bit instrs: DL, DMB, DSB. ISM. MRS, MSR

In ARM.td, FeatureDB has already been defined for Armv6m.
However, when existing code actually checks for availability of atomic feature (fence + ldrex/strex), if the target is Thumb but not supports ARMv8mBaseline, then it assumes no such feature.

For example: in ARMISelLowering.cpp::1043

if (Subtarget->hasAnyDataBarrier() &&
    (!Subtarget->isThumb() || Subtarget->hasV8MBaselineOps())
bool ARMSubtarget::enableAtomicExpand() const {
  return hasAnyDataBarrier();
}
TargetLowering::AtomicExpansionKind
ARMTargetLowering::shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const {
  unsigned Size = AI->getType()->getPrimitiveSizeInBits();
  return ((!isThumb() || hasV8MBaselineOps()) && Size <= (Subtarget->isMClass() ? 32U : 64U))
             ? AtomicExpansionKind::LLSC
             : AtomicExpansionKind::None;
}

Is this not sufficient for some reason?

Agree with the change in shouldExpandAtomicRMWInIR but for ARMSubtarget::enableAtomicExpand(),
changing from

return hasAnyDataBarrier() && (!isThumb() || hasV8MBaselineOps()

to

return hasAnyDataBarrier();

Is it too relax? Seems OK but not very sure.

Should be fine; the whole point of this patch is that you want to run atomic expansion for targets which have dmb, but not ldrex/strex.

weimingz updated this revision to Diff 76500.Oct 31 2016, 4:09 PM
weimingz edited edge metadata.

Fix the test case and remove isARMv6m()

Looks fine, but someone more familiar with the ARM backend should double-check I didn't miss anything.

jmolloy requested changes to this revision.Nov 3 2016, 2:14 AM
jmolloy edited edge metadata.
jmolloy added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
12879

What about ARMv7-A/R here? Previously we'd return LLSC in this case.

12892

Same here - ARMv7-AR?

This revision now requires changes to proceed.Nov 3 2016, 2:14 AM
weimingz added inline comments.Nov 3 2016, 2:40 PM
lib/Target/ARM/ARMISelLowering.cpp
12879

ARMv7-A/R should not be affected because they have V8MBaselineOps() (inherits from v6t2)

jmolloy accepted this revision.Nov 3 2016, 2:42 PM
jmolloy edited edge metadata.

Ah, sorry, I'd missed that!

LGTM, cheers!

This revision was automatically updated to reflect the committed changes.