maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

[InstCombine] Do not combine atomic and non-atomic loads 

Before this change, InstCombine was willing to fold atomic and
non-atomic loads through a PHI node as long as the first PHI argument
is not an atomic load. The combined load would be non-atomic, which is
incorrect.

Fix this by only combining the loads in a PHI node when all of the
arguments are non-atomic loads.

Thanks to Eli Friedman for pointing out the bug at
https://github.com/llvm/llvm-project/issues/50777#issuecomment-981045342!

Fixes #50777

Differential Revision: https://reviews.llvm.org/D115113
This commit is contained in:
Ricky Zhou 2022-01-30 10:05:11 -05:00 committed by Sanjay Patel
parent 2d283528ba
commit 30ac5f9e64
2 changed files with 11 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
View File

@ -666,7 +666,7 @@ Instruction *InstCombinerImpl::foldPHIArgLoadIntoPHI(PHINode &PN) {
// sunk load: whether it is volatile, and what its alignment is. // sunk load: whether it is volatile, and what its alignment is.
bool IsVolatile = FirstLI->isVolatile(); bool IsVolatile = FirstLI->isVolatile();
Align LoadAlignment = FirstLI->getAlign(); Align LoadAlignment = FirstLI->getAlign();
unsigned LoadAddrSpace = FirstLI->getPointerAddressSpace(); const unsigned LoadAddrSpace = FirstLI->getPointerAddressSpace();
// We can't sink the load if the loaded value could be modified between the // We can't sink the load if the loaded value could be modified between the
// load and the PHI. // load and the PHI.
@ -681,19 +681,21 @@ Instruction *InstCombinerImpl::foldPHIArgLoadIntoPHI(PHINode &PN) {
FirstLI->getParent()->getTerminator()->getNumSuccessors() != 1) FirstLI->getParent()->getTerminator()->getNumSuccessors() != 1)
return nullptr; return nullptr;
// Check to see if all arguments are the same operation.
for (auto Incoming : drop_begin(zip(PN.blocks(), PN.incoming_values()))) { for (auto Incoming : drop_begin(zip(PN.blocks(), PN.incoming_values()))) {
BasicBlock *InBB = std::get<0>(Incoming); BasicBlock *InBB = std::get<0>(Incoming);
Value *InVal = std::get<1>(Incoming); Value *InVal = std::get<1>(Incoming);
LoadInst *LI = dyn_cast<LoadInst>(InVal); LoadInst *LI = dyn_cast<LoadInst>(InVal);
if (!LI || !LI->hasOneUser()) if (!LI || !LI->hasOneUser() || LI->isAtomic())
return nullptr;
// Make sure all arguments are the same type of operation.
if (LI->isVolatile() != IsVolatile ||
LI->getPointerAddressSpace() != LoadAddrSpace)
return nullptr; return nullptr;
// We can't sink the load if the loaded value could be modified between // We can't sink the load if the loaded value could be modified between
// the load and the PHI. // the load and the PHI.
if (LI->isVolatile() != IsVolatile || LI->getParent() != InBB || if (LI->getParent() != InBB || !isSafeAndProfitableToSinkLoad(LI))
LI->getPointerAddressSpace() != LoadAddrSpace ||
!isSafeAndProfitableToSinkLoad(LI))
return nullptr; return nullptr;
LoadAlignment = std::min(LoadAlignment, LI->getAlign()); LoadAlignment = std::min(LoadAlignment, LI->getAlign());

8
llvm/test/Transforms/InstCombine/phi.ll
View File

@ -547,17 +547,15 @@ bb2: ; preds = %bb1, %entry
ret i32 %res ret i32 %res
} }
; FIXME: Atomic and non-atomic loads should not be combined. ; Atomic and non-atomic loads should not be combined.
define i32 @PR51435(i32* %ptr, i32* %atomic_ptr, i1 %c) { define i32 @PR51435(i32* %ptr, i32* %atomic_ptr, i1 %c) {
; CHECK-LABEL: @PR51435( ; CHECK-LABEL: @PR51435(
; CHECK: entry: ; CHECK: entry:
; CHECK-NEXT: br i1 %c, label %if, label %end ; CHECK-NEXT: [[NON_ATOMIC:%.*]] = load i32, i32* %ptr, align 4
; CHECK: if: ; CHECK: if:
; CHECK-NEXT: [[ATOMIC:%.*]] = load atomic i32, i32* %atomic_ptr acquire, align 4 ; CHECK-NEXT: [[ATOMIC:%.*]] = load atomic i32, i32* %atomic_ptr acquire, align 4
; CHECK-NEXT: br label %end
; CHECK: end: ; CHECK: end:
; CHECK-NEXT: [[COND_IN:%.*]] = phi i32* [ %ptr, %entry ], [ %atomic_ptr, %if ] ; CHECK-NEXT: [[COND:%.*]] = phi i32 [ [[NON_ATOMIC]], %entry ], [ [[ATOMIC]], %if ]
; CHECK-NEXT: [[COND:%.*]] = load i32, i32* [[COND_IN]], align 4
; CHECK-NEXT: ret i32 [[COND]] ; CHECK-NEXT: ret i32 [[COND]]
entry: entry:
%x = load i32, i32* %ptr, align 4 %x = load i32, i32* %ptr, align 4