Codegen support for 128-bit atomicrmw (and|or|xchg).
- store atomic -> swpp
- atomicrmw xchg -> swpp
- atomicrmw and -> ldclrp
- atomicrmw or -> ldsetp
Details
Details
- Reviewers
lenary stuij pratlucas - Commits
- rGf1f583347d00: [AArch64] Codegen for FEAT_LSE128
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
| Time | Test | |
|---|---|---|
| 60,090 ms | x64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp Script:
--
: 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp
|
Event Timeline
| llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
|---|---|---|
| 22098 | I am also confused about this assert message. | |
| 22281 | The message here doesn't correspond to the case? | |
| llvm/test/CodeGen/AArch64/Atomics/aarch64-atomic-store-lse128.ll | ||
| 119 | I thought we tested O0 because of globalisel, but here both O0 and O1 are updated, despite not having changed globalisel. Why? | |
| llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
|---|---|---|
| 22098 | See ReplaceATOMIC_LOAD_128Results which has more explanation. It will convert ATOMIC_LOAD_AND to 2x XOR + LDCLRP (determined by this function, getAtomicLoad128Opcode). This is in contrast to other sizes, which convert ATOMIC_LOAD_AND to ATOMIC_LOAD_CLR earlier, because i128 is not legal. The assert is checking that no ATOMIC_LOAD_CLR made it through, because for 128 we should see only ATOMIC_LOAD_AND here. | |
| 22281 | This assert is also checking that no 128-bit ATOMIC_LOAD_CLR appeared, which would indicate that ATOMIC_LOAD_AND was not lowered directly to a MachineInstr. | |
| llvm/test/CodeGen/AArch64/Atomics/aarch64-atomic-store-lse128.ll | ||
| 119 | GlobalISel does not support quite a lot of the atomics, so it is falling back to SelectionDAG. This is also the reason so many of the tests have identical output for O0 and O1. The idea with using optimisation levels rather than forcing -global-isel is that what we care about at the end of the day is the actual output, rather than how complete GlobalISel is. | |
| llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
|---|---|---|
| 22098 | Ok, I see, and yet I worry I'd forget this again looking directly at the assert message, which never mentions _CLR (the link between _AND and _CLR seems implicit to me) | |
| 22281 | I think it might be clearer if the assert message said "ATOMIC_LOAD_CLR should be legal" maybe? | |
| llvm/test/CodeGen/AArch64/Atomics/aarch64-atomic-store-lse128.ll | ||
| 119 | Right, ok. This does seem like something we could improve, but it's fine to happen later. | |
Comment Actions
Right now, GlobalISel falls back to SelectionDAG for many atomics, including those above (the -O0 case is the globalisel case, but it has fallen back). I think GlobalISel work for these atomics can come in a later patch, to be honest.
Comment Actions
No, FEAT_LSE128 only implies FEAT_LSE, not FEAT_LSE2.
In that case, can you add a few tests to show what happens on targets that have both lse2 and lse128? I assume lse2 "wins", but it would be nice to make that explicit.