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

[rickyz]

Summary

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

[spatel]

Can we reduce the test to something like this and add it to the file where this transform is tested more generally (llvm/test/Transforms/InstCombine/phi.ll?):

define i32 @PR51435(i32* %ptr, i32* %atomic_ptr, i1 %c) {
entry:
  %x = load i32, i32* %ptr, align 4
  br i1 %c, label %if, label %end

if:
  %y = load atomic i32, i32* %atomic_ptr acquire, align 4
  br label %end

end:
  %cond = phi i32 [ %x, %entry ], [ %y, %if ]
  ret i32 %cond
}

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

663 ↗

(On Diff #391907)

To conform with current coding standards, this should be "IsAtomic". Fix the existing code as an NFC pre-commit?

[rickyz]

Respond to comments.

[rickyz]

Thanks for the reduced test case - I verified that it fails before this change and passes after.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

663 ↗

(On Diff #391907)

I went through the existing variables violating the coding standards and fixed them - most of them were loop iterators that I converted to range for loops. That change is unfortunately a little larger than I'd hoped - I split those changes into D116086.

[nikic]

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

699 ↗

(On Diff #395600)

I think you'd be better of just bailing on LI->isAtomic() here, just as the check on the initial load does. IsAtomic is always false here, and if that weren't the case, then this check would be insufficient, because it conflates all types of atomic loads, which might have different ordering or sync scope.

[rickyz]

Make it clearer that we give up immediately on encountering an atomic load.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

699 ↗

(On Diff #395600)

Agreed, thanks!

[spatel]

LGTM. We could move some of the cosmetic diffs ("const" etc) into D116086 or another pre-commit, but this seems fine too.

llvm/test/Transforms/InstCombine/phi.ll

550–551

Please pre-commit this test to 'main' with a FIXME comment, so we have a record of the bug in a regression test.

[rickyz]

Rebasing on top of D118554.

llvm/test/Transforms/InstCombine/phi.ll

 
 bb2:        ; preds = %bb1, %entry
   %cond = phi i1 [ true, %bb1 ], [ false, %entry ]        ; <i1> [#uses=1]
   %val = phi i32 [ %0, %bb1 ], [ 0, %entry ]        ; <i32> [#uses=1]
   %res = select i1 %cond, i32 %val, i32 0        ; <i32> [#uses=1]
   ret i32 %res
 }
 
+; FIXME: Atomic and non-atomic loads should not be combined.
+define i32 @PR51435(i32* %ptr, i32* %atomic_ptr, i1 %c) {
+; CHECK-LABEL: @PR51435(
+; CHECK:       entry:
+; CHECK-NEXT:    br i1 %c, label %if, label %end
+; CHECK:       if:
+; CHECK-NEXT:    [[ATOMIC:%.*]] = load atomic i32, i32* %atomic_ptr acquire, align 4
+; CHECK-NEXT:    br label %end
+; CHECK:       end:
+; CHECK-NEXT:    [[COND_IN:%.*]] = phi i32* [ %ptr, %entry ], [ %atomic_ptr, %if ]
+; CHECK-NEXT:    [[COND:%.*]] = load i32, i32* [[COND_IN]], align 4
+; CHECK-NEXT:    ret i32 [[COND]]
+entry:
+  %x = load i32, i32* %ptr, align 4
+  br i1 %c, label %if, label %end
+
+if:
+  %y = load atomic i32, i32* %atomic_ptr acquire, align 4
+  br label %end
+
+end:
+  %cond = phi i32 [ %x, %entry ], [ %y, %if ]
+  ret i32 %cond
+}
+
 define i1 @test18(i1 %cond) {
 ; CHECK-LABEL: @test18(
 ; CHECK-NEXT:    br i1 [[COND:%.*]], label [[TRUE:%.*]], label [[FALSE:%.*]]
 ; CHECK:       true:
 ; CHECK-NEXT:    br label [[RET:%.*]]
 ; CHECK:       false:
 ; CHECK-NEXT:    br label [[RET]]
 ; CHECK:       ret: