This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Prevent spilling between ldxr/stxr pairs
ClosedPublic

Authored by LemonBoy on Apr 23 2021, 7:16 AM.

Details

Summary

Apply the same logic used to check if CMPXCHG nodes should be expanded at -O0: the register allocator may end up spilling some register in between the atomic load/store pairs, breaking the atomicity and possibly stalling the execution.

Fixes PR48017

Diff Detail

Event Timeline

LemonBoy created this revision.Apr 23 2021, 7:16 AM
LemonBoy requested review of this revision.Apr 23 2021, 7:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2021, 7:16 AM

The same problem exists in the ARM backend, which I fixed internally by implementing new pseudos for the atomicrmw instructions, but hadn't got round to upstreaming it yet. Here it is for comparison: D101164.

LemonBoy updated this revision to Diff 340247.Apr 24 2021, 12:38 AM

Run test at -O1 so that the check pattern remains unchanged.

The same problem exists in the ARM backend, which I fixed internally by implementing new pseudos for the atomicrmw instructions, but hadn't got round to upstreaming it yet. Here it is for comparison: D101164.

That sounds like proper way of fixing this problem and produce tighter code.
This patch (or D101216, cc @efriedma) is IMO a good stopgap solution to avoid miscompiling rmw ops that can possibly land in LLVM 12.0.1.

I don't think that patch addresses this issue.

llvm/test/CodeGen/AArch64/atomicrmw-O0.ll
3

Please add a RUN line with LSE enabled.

Please add a test for "nand".

llvm/test/Transforms/AtomicExpand/AArch64/expand-atomicrmw-xchg-fp.ll
3

I don't think this change is necessary?

LemonBoy updated this revision to Diff 341780.Apr 29 2021, 11:43 PM

Add atomicrmw/nand test, check the output when LSE is enabled.

LemonBoy marked an inline comment as done.Apr 29 2021, 11:48 PM
LemonBoy added inline comments.
llvm/test/Transforms/AtomicExpand/AArch64/expand-atomicrmw-xchg-fp.ll
3

At O0 there's no llvm.aarch64.ldaxr.p0f16 nor stxr being emitted after this patch, I've left the test unchanged by bumping up the opt level.

efriedma accepted this revision.Apr 30 2021, 10:44 AM

LGTM with one minor comment.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16695

"implement cmpxchg" doesn't seem right; should be "implement atomicrmw"? (Also "address being exchanged".)

llvm/test/Transforms/AtomicExpand/AArch64/expand-atomicrmw-xchg-fp.ll
3

Oh, I see, this is opt, not llc. I guess it's fine.

This revision is now accepted and ready to land.Apr 30 2021, 10:44 AM
This revision was landed with ongoing or failed builds.May 1 2021, 8:17 AM
This revision was automatically updated to reflect the committed changes.