Skip to content

Commit 288a95f

Browse files
preamespreames
committedFeb 27, 2019
Seperate volatility and atomicity/ordering in SelectionDAG
At the moment, we mark every atomic memory access as being also volatile. This is unnecessarily conservative and prohibits many legal transforms (DCE, folding, etc..). This patch removes MOVolatile from the MachineMemOperands of atomic, but not volatile, instructions. This should be strictly NFC after a series of previous patches which have gone in to ensure backend code is conservative about handling of isAtomic MMOs. Once it's in and baked for a bit, we'll start working through removing unnecessary bailouts one by one. We applied this same strategy to the middle end a few years ago, with good success. To make sure this patch itself is NFC, it is build on top of a series of other patches which adjust code to (for the moment) be as conservative for an atomic access as for a volatile access and build up a test corpus (mostly in test/CodeGen/X86/atomics-unordered.ll).. Previously landed D57593 Fix a bug in the definition of isUnordered on MachineMemOperand D57596 [CodeGen] Be conservative about atomic accesses as for volatile D57802 Be conservative about unordered accesses for the moment rL353959: [Tests] First batch of cornercase tests for unordered atomics. rL353966: [Tests] RMW folding tests w/unordered atomic operations. rL353972: [Tests] More unordered atomic lowering tests. rL353989: [SelectionDAG] Inline a single use helper function, and remove last non-MMO interface rL354740: [Hexagon, SystemZ] Be super conservative about atomics rL354800: [Lanai] Be super conservative about atomics rL354845: [ARM] Be super conservative about atomics Attention Out of Tree Backend Owners: This patch may break you. If it does, you can use the TLI getMMOFlags hook to restore the MOVolatile to any instruction you need to. (See llvm-dev thread titled "PSA: Changes to how atomics are handled in backends" started Feb 27, 2019.) Differential Revision: https://reviews.llvm.org/D57601 llvm-svn: 355025
1 parent c526e02 commit 288a95f

File tree

6 files changed

+66
-21
lines changed

6 files changed

+66
-21
lines changed
 

‎llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4417,10 +4417,10 @@ void SelectionDAGBuilder::visitAtomicCmpXchg(const AtomicCmpXchgInst &I) {
44174417

44184418
auto Alignment = DAG.getEVTAlignment(MemVT);
44194419

4420-
// FIXME: Volatile isn't really correct; we should keep track of atomic
4421-
// orderings in the memoperand.
4422-
auto Flags = MachineMemOperand::MOVolatile | MachineMemOperand::MOLoad |
4423-
MachineMemOperand::MOStore;
4420+
auto Flags = MachineMemOperand::MOLoad | MachineMemOperand::MOStore;
4421+
if (I.isVolatile())
4422+
Flags |= MachineMemOperand::MOVolatile;
4423+
Flags |= DAG.getTargetLoweringInfo().getMMOFlags(I);
44244424

44254425
MachineFunction &MF = DAG.getMachineFunction();
44264426
MachineMemOperand *MMO =
@@ -4468,12 +4468,10 @@ void SelectionDAGBuilder::visitAtomicRMW(const AtomicRMWInst &I) {
44684468
auto MemVT = getValue(I.getValOperand()).getSimpleValueType();
44694469
auto Alignment = DAG.getEVTAlignment(MemVT);
44704470

4471-
// For now, atomics are considered to be volatile always, and they are
4472-
// chained as such.
4473-
// FIXME: Volatile isn't really correct; we should keep track of atomic
4474-
// orderings in the memoperand.
4475-
auto Flags = MachineMemOperand::MOVolatile |
4476-
MachineMemOperand::MOLoad | MachineMemOperand::MOStore;
4471+
auto Flags = MachineMemOperand::MOLoad | MachineMemOperand::MOStore;
4472+
if (I.isVolatile())
4473+
Flags |= MachineMemOperand::MOVolatile;
4474+
Flags |= DAG.getTargetLoweringInfo().getMMOFlags(I);
44774475

44784476
MachineFunction &MF = DAG.getMachineFunction();
44794477
MachineMemOperand *MMO =
@@ -4518,12 +4516,15 @@ void SelectionDAGBuilder::visitAtomicLoad(const LoadInst &I) {
45184516
I.getAlignment() < VT.getStoreSize())
45194517
report_fatal_error("Cannot generate unaligned atomic load");
45204518

4519+
auto Flags = MachineMemOperand::MOLoad;
4520+
if (I.isVolatile())
4521+
Flags |= MachineMemOperand::MOVolatile;
4522+
Flags |= TLI.getMMOFlags(I);
4523+
45214524
MachineMemOperand *MMO =
45224525
DAG.getMachineFunction().
45234526
getMachineMemOperand(MachinePointerInfo(I.getPointerOperand()),
4524-
MachineMemOperand::MOVolatile |
4525-
MachineMemOperand::MOLoad,
4526-
VT.getStoreSize(),
4527+
Flags, VT.getStoreSize(),
45274528
I.getAlignment() ? I.getAlignment() :
45284529
DAG.getEVTAlignment(VT),
45294530
AAMDNodes(), nullptr, SSID, Order);
@@ -4554,11 +4555,10 @@ void SelectionDAGBuilder::visitAtomicStore(const StoreInst &I) {
45544555
if (I.getAlignment() < VT.getStoreSize())
45554556
report_fatal_error("Cannot generate unaligned atomic store");
45564557

4557-
// For now, atomics are considered to be volatile always, and they are
4558-
// chained as such.
4559-
// FIXME: Volatile isn't really correct; we should keep track of atomic
4560-
// orderings in the memoperand.
4561-
auto Flags = MachineMemOperand::MOVolatile | MachineMemOperand::MOStore;
4558+
auto Flags = MachineMemOperand::MOStore;
4559+
if (I.isVolatile())
4560+
Flags |= MachineMemOperand::MOVolatile;
4561+
Flags |= TLI.getMMOFlags(I);
45624562

45634563
MachineFunction &MF = DAG.getMachineFunction();
45644564
MachineMemOperand *MMO =

‎llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3718,6 +3718,27 @@ SDValue SystemZTargetLowering::lowerATOMIC_CMP_SWAP(SDValue Op,
37183718
return SDValue();
37193719
}
37203720

3721+
MachineMemOperand::Flags
3722+
SystemZTargetLowering::getMMOFlags(const Instruction &I) const {
3723+
// Because of how we convert atomic_load and atomic_store to normal loads and
3724+
// stores in the DAG, we need to ensure that the MMOs are marked volatile
3725+
// since DAGCombine hasn't been updated to account for atomic, but non
3726+
// volatile loads. (See D57601)
3727+
if (auto *SI = dyn_cast<StoreInst>(&I))
3728+
if (SI->isAtomic())
3729+
return MachineMemOperand::MOVolatile;
3730+
if (auto *LI = dyn_cast<LoadInst>(&I))
3731+
if (LI->isAtomic())
3732+
return MachineMemOperand::MOVolatile;
3733+
if (auto *AI = dyn_cast<AtomicRMWInst>(&I))
3734+
if (AI->isAtomic())
3735+
return MachineMemOperand::MOVolatile;
3736+
if (auto *AI = dyn_cast<AtomicCmpXchgInst>(&I))
3737+
if (AI->isAtomic())
3738+
return MachineMemOperand::MOVolatile;
3739+
return MachineMemOperand::MONone;
3740+
}
3741+
37213742
SDValue SystemZTargetLowering::lowerSTACKSAVE(SDValue Op,
37223743
SelectionDAG &DAG) const {
37233744
MachineFunction &MF = DAG.getMachineFunction();

‎llvm/lib/Target/SystemZ/SystemZISelLowering.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -642,6 +642,7 @@ class SystemZTargetLowering : public TargetLowering {
642642
MachineBasicBlock *MBB,
643643
unsigned Opcode) const;
644644

645+
MachineMemOperand::Flags getMMOFlags(const Instruction &I) const override;
645646
const TargetRegisterClass *getRepRegClassFor(MVT VT) const override;
646647
};
647648

‎llvm/lib/Target/XCore/XCoreISelLowering.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1008,6 +1008,27 @@ LowerATOMIC_STORE(SDValue Op, SelectionDAG &DAG) const {
10081008
return SDValue();
10091009
}
10101010

1011+
MachineMemOperand::Flags
1012+
XCoreTargetLowering::getMMOFlags(const Instruction &I) const {
1013+
// Because of how we convert atomic_load and atomic_store to normal loads and
1014+
// stores in the DAG, we need to ensure that the MMOs are marked volatile
1015+
// since DAGCombine hasn't been updated to account for atomic, but non
1016+
// volatile loads. (See D57601)
1017+
if (auto *SI = dyn_cast<StoreInst>(&I))
1018+
if (SI->isAtomic())
1019+
return MachineMemOperand::MOVolatile;
1020+
if (auto *LI = dyn_cast<LoadInst>(&I))
1021+
if (LI->isAtomic())
1022+
return MachineMemOperand::MOVolatile;
1023+
if (auto *AI = dyn_cast<AtomicRMWInst>(&I))
1024+
if (AI->isAtomic())
1025+
return MachineMemOperand::MOVolatile;
1026+
if (auto *AI = dyn_cast<AtomicCmpXchgInst>(&I))
1027+
if (AI->isAtomic())
1028+
return MachineMemOperand::MOVolatile;
1029+
return MachineMemOperand::MONone;
1030+
}
1031+
10111032
//===----------------------------------------------------------------------===//
10121033
// Calling Convention Implementation
10131034
//===----------------------------------------------------------------------===//

‎llvm/lib/Target/XCore/XCoreISelLowering.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,8 @@ namespace llvm {
188188
SDValue LowerATOMIC_LOAD(SDValue Op, SelectionDAG &DAG) const;
189189
SDValue LowerATOMIC_STORE(SDValue Op, SelectionDAG &DAG) const;
190190

191+
MachineMemOperand::Flags getMMOFlags(const Instruction &I) const override;
192+
191193
// Inline asm support
192194
std::pair<unsigned, const TargetRegisterClass *>
193195
getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,

‎llvm/test/CodeGen/AMDGPU/syncscopes.ll

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx803 -stop-after=si-insert-skips < %s | FileCheck --check-prefix=GCN %s
22

33
; GCN-LABEL: name: syncscopes
4-
; GCN: FLAT_STORE_DWORD killed renamable $vgpr1_vgpr2, killed renamable $vgpr0, 0, 0, 0, implicit $exec, implicit $flat_scr :: (volatile store syncscope("agent") seq_cst 4 into %ir.agent_out)
5-
; GCN: FLAT_STORE_DWORD killed renamable $vgpr4_vgpr5, killed renamable $vgpr3, 0, 0, 0, implicit $exec, implicit $flat_scr :: (volatile store syncscope("workgroup") seq_cst 4 into %ir.workgroup_out)
6-
; GCN: FLAT_STORE_DWORD killed renamable $vgpr7_vgpr8, killed renamable $vgpr6, 0, 0, 0, implicit $exec, implicit $flat_scr :: (volatile store syncscope("wavefront") seq_cst 4 into %ir.wavefront_out)
4+
; GCN: FLAT_STORE_DWORD killed renamable $vgpr1_vgpr2, killed renamable $vgpr0, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store syncscope("agent") seq_cst 4 into %ir.agent_out)
5+
; GCN: FLAT_STORE_DWORD killed renamable $vgpr4_vgpr5, killed renamable $vgpr3, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store syncscope("workgroup") seq_cst 4 into %ir.workgroup_out)
6+
; GCN: FLAT_STORE_DWORD killed renamable $vgpr7_vgpr8, killed renamable $vgpr6, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store syncscope("wavefront") seq_cst 4 into %ir.wavefront_out)
77
define void @syncscopes(
88
i32 %agent,
99
i32* %agent_out,

0 commit comments

Comments
 (0)
Please sign in to comment.