Skip to content

Commit

Permalink
[InstCombine] Check FPMathOperator for Ctx before FMF check
Browse files Browse the repository at this point in the history 
We need to check FPMathOperator for Ctx instruction before checking fast
math flag on this Ctx.

Ctx is not always an FPMathOperator, so explicitly check for it.

Fixes #71548.
  • Loading branch information
@annamthomas
annamthomas committed Nov 7, 2023
1 parent 551c280 commit f0cdf4b
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 1 deletion.
3 changes: 2 additions & 1 deletion llvm/lib/Analysis/InstructionSimplify.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6606,7 +6606,8 @@ static Value *simplifyBinaryIntrinsic(Function *F, Value *Op0, Value *Op1,
// float, if the ninf flag is set.
const APFloat *C;
if (match(Op1, m_APFloat(C)) &&
(C->isInfinity() || (Q.CxtI->hasNoInfs() && C->isLargest()))) {
(C->isInfinity() || (isa<FPMathOperator>(Q.CxtI) &&
Q.CxtI->hasNoInfs() && C->isLargest()))) {

This comment has been minimized.

Sign in to view

Copy link
@nikic

nikic Nov 7, 2023

Contributor

I don't think this is the right fix. This code is just wrong. Q.CxtI is some random instruction, it's not necessarily the minnum call being simplified. As such, checking flags on it is meaningless.

What we need to do instead is to pass down the Call from simplifyIntrinsic() and then check the flags on that.

This comment has been minimized.

Sign in to view

Copy link
@nikic

nikic Nov 7, 2023

Contributor

(Or alternatively, explicitly pass down the FMF from the call, if any.)

This comment has been minimized.

Sign in to view

Copy link
@annamthomas

annamthomas Nov 7, 2023

Author Contributor

Yeah, probably the same testcase can be used to show CxtI can be any instruction. I'll make a fix. Thanks! Was just closing the bug to fix the existing crash =)
// minnum(X, -inf) -> -inf
// maxnum(X, +inf) -> +inf
// minimum(X, -inf) -> -inf if nnan
Expand Down
26 changes: 26 additions & 0 deletions llvm/test/Transforms/InstCombine/minimum.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,3 +471,29 @@ define double @negated_op_extra_use(double %x) {
%r = call double @llvm.minimum.f64(double %negx, double %x)
ret double %r
}

; Testcase from PR 71548.
define void @pr71548() {
; CHECK-LABEL: @pr71548(
; CHECK-NEXT: [[C0:%.*]] = load atomic double, ptr addrspace(1) null unordered, align 8
; CHECK-NEXT: [[C1:%.*]] = load atomic i32, ptr addrspace(1) null unordered, align 4
; CHECK-NEXT: [[C2:%.*]] = sitofp i32 [[C1]] to double
; CHECK-NEXT: [[CRES_I:%.*]] = call noundef double @llvm.minimum.f64(double [[C0]], double [[C2]])
; CHECK-NEXT: [[C3:%.*]] = fcmp ult double [[CRES_I]], 0.000000e+00
; CHECK-NEXT: [[C_NOT16:%.*]] = icmp eq i32 [[C1]], 0
; CHECK-NEXT: [[COR_COND45:%.*]] = or i1 [[C3]], [[C_NOT16]]
; CHECK-NEXT: call void @llvm.assume(i1 [[COR_COND45]])
; CHECK-NEXT: ret void
;
%c0 = load atomic double, ptr addrspace(1) null unordered, align 8
%c1 = load atomic i32, ptr addrspace(1) null unordered, align 4
%c2 = sitofp i32 %c1 to double
%cres.i = call noundef double @llvm.minimum.f64(double %c0, double %c2)
%c3 = fcmp ult double %cres.i, 0.000000e+00
%c.not16 = icmp eq i32 %c1, 0
%cor.cond45 = or i1 %c3, %c.not16
call void @llvm.assume(i1 %cor.cond45)
ret void
}

declare void @llvm.assume(i1)

0 comments on commit f0cdf4b

Please sign in to comment.