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
30 msx64 debian > LLVM.Transforms/AtomicExpand/AArch64::expand-atomicrmw-xchg-fp.ll
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt -S -mtriple=aarch64-- -atomic-expand /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/AtomicExpand/AArch64/expand-atomicrmw-xchg-fp.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/AtomicExpand/AArch64/expand-atomicrmw-xchg-fp.ll
160 msx64 windows > LLVM.CodeGen/AMDGPU::wwm-reserved-spill.ll
Script: -- : 'RUN: at line 2'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\llc.exe -O0 -march=amdgcn -mcpu=gfx900 -amdgpu-dpp-combine=false -verify-machineinstrs < C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\CodeGen\AMDGPU\wwm-reserved-spill.ll | c:\ws\w16-1\llvm-project\premerge-checks\build\bin\filecheck.exe -check-prefixes=GFX9,GFX9-O0 C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\CodeGen\AMDGPU\wwm-reserved-spill.ll
70 msx64 windows > LLVM.Transforms/AtomicExpand/AArch64::expand-atomicrmw-xchg-fp.ll
Script: -- : 'RUN: at line 2'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\opt.exe -S -mtriple=aarch64-- -atomic-expand C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\Transforms\AtomicExpand\AArch64\expand-atomicrmw-xchg-fp.ll | c:\ws\w16-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\Transforms\AtomicExpand\AArch64\expand-atomicrmw-xchg-fp.ll

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 ↗(On Diff #340247)

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 ↗(On Diff #340247)

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
16675

"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 ↗(On Diff #340247)

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.