Page MenuHomePhabricator

[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

Unit TestsFailed

TimeTest
80 msx64 debian > Clang.Driver::debug-pass-structure.c
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -fexperimental-new-pass-manager -fdebug-pass-structure -O3 -S -emit-llvm /mnt/disks/ssd0/agent/llvm-project/clang/test/Driver/debug-pass-structure.c -o /dev/null 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/Driver/debug-pass-structure.c --check-prefix=NEWPM
110 msx64 debian > LLVM.CodeGen/AArch64/GlobalISel::arm64-atomic.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/arm64-atomic.ll -mtriple=arm64-apple-ios -global-isel -global-isel-abort=1 -verify-machineinstrs | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/arm64-atomic.ll --check-prefixes=CHECK-NOLSE,CHECK-NOLSE-O1
200 msx64 windows > LLVM.CodeGen/AArch64/GlobalISel::arm64-atomic.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-2\llvm-project\premerge-checks\build\bin\llc.exe < C:\ws\w16c2-2\llvm-project\premerge-checks\llvm\test\CodeGen\AArch64\GlobalISel\arm64-atomic.ll -mtriple=arm64-apple-ios -global-isel -global-isel-abort=1 -verify-machineinstrs | c:\ws\w16c2-2\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-2\llvm-project\premerge-checks\llvm\test\CodeGen\AArch64\GlobalISel\arm64-atomic.ll --check-prefixes=CHECK-NOLSE,CHECK-NOLSE-O1

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
16691

"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.