<div dir="ltr"><div>It was a copy-paste error on my part - missed the null check for the common operand, so we just crashed on any pattern with 4 unique min/max operands.</div><div><br></div><div>See the negative test here:</div><div><a href="https://reviews.llvm.org/rG14eefa57f2b6">https://reviews.llvm.org/rG14eefa57f2b6</a></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 12, 2021 at 3:42 PM Roman Lebedev via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Reproducer?<br>
<br>
On Thu, Aug 12, 2021 at 10:36 PM Amy Huang via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
><br>
> Author: Amy Huang<br>
> Date: 2021-08-12T12:36:25-07:00<br>
> New Revision: 427520a8fa09cc9e65e3ee118be7cc5f1ac21d10<br>
><br>
> URL: <a href="https://github.com/llvm/llvm-project/commit/427520a8fa09cc9e65e3ee118be7cc5f1ac21d10" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/427520a8fa09cc9e65e3ee118be7cc5f1ac21d10</a><br>
> DIFF: <a href="https://github.com/llvm/llvm-project/commit/427520a8fa09cc9e65e3ee118be7cc5f1ac21d10.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/427520a8fa09cc9e65e3ee118be7cc5f1ac21d10.diff</a><br>
><br>
> LOG: Revert "[InstCombine] factorize min/max intrinsic ops with common operand"<br>
><br>
> This reverts commit 6de1dbbd09c12abbec7eb187ffa1afbd47302dfa because it causes a<br>
> compiler crash.<br>
><br>
> Added:<br>
><br>
><br>
> Modified:<br>
> llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> llvm/test/Transforms/InstCombine/minmax-intrinsics.ll<br>
><br>
> Removed:<br>
><br>
><br>
><br>
> ################################################################################<br>
> diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> index d6ccf0fcc708..210652e23377 100644<br>
> --- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> +++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> @@ -795,60 +795,6 @@ static Instruction *foldClampRangeOfTwo(IntrinsicInst *II,<br>
> return SelectInst::Create(Cmp, ConstantInt::get(II->getType(), *C0), I1);<br>
> }<br>
><br>
> -/// Reduce a sequence of min/max intrinsics with a common operand.<br>
> -static Instruction *factorizeMinMaxTree(IntrinsicInst *II) {<br>
> - // Match 3 of the same min/max ops. Example: umin(umin(), umin()).<br>
> - auto *LHS = dyn_cast<IntrinsicInst>(II->getArgOperand(0));<br>
> - auto *RHS = dyn_cast<IntrinsicInst>(II->getArgOperand(1));<br>
> - Intrinsic::ID MinMaxID = II->getIntrinsicID();<br>
> - if (!LHS || !RHS || LHS->getIntrinsicID() != MinMaxID ||<br>
> - RHS->getIntrinsicID() != MinMaxID ||<br>
> - (!LHS->hasOneUse() && !RHS->hasOneUse()))<br>
> - return nullptr;<br>
> -<br>
> - Value *A = LHS->getArgOperand(0);<br>
> - Value *B = LHS->getArgOperand(1);<br>
> - Value *C = RHS->getArgOperand(0);<br>
> - Value *D = RHS->getArgOperand(1);<br>
> -<br>
> - // Look for a common operand.<br>
> - Value *MinMaxOp = nullptr;<br>
> - Value *ThirdOp = nullptr;<br>
> - if (LHS->hasOneUse()) {<br>
> - // If the LHS is only used in this chain and the RHS is used outside of it,<br>
> - // reuse the RHS min/max because that will eliminate the LHS.<br>
> - if (D == A || C == A) {<br>
> - // min(min(a, b), min(c, a)) --> min(min(c, a), b)<br>
> - // min(min(a, b), min(a, d)) --> min(min(a, d), b)<br>
> - MinMaxOp = RHS;<br>
> - ThirdOp = B;<br>
> - } else if (D == B || C == B) {<br>
> - // min(min(a, b), min(c, b)) --> min(min(c, b), a)<br>
> - // min(min(a, b), min(b, d)) --> min(min(b, d), a)<br>
> - MinMaxOp = RHS;<br>
> - ThirdOp = A;<br>
> - }<br>
> - } else {<br>
> - assert(RHS->hasOneUse() && "Expected one-use operand");<br>
> - // Reuse the LHS. This will eliminate the RHS.<br>
> - if (D == A || D == B) {<br>
> - // min(min(a, b), min(c, a)) --> min(min(a, b), c)<br>
> - // min(min(a, b), min(c, b)) --> min(min(a, b), c)<br>
> - MinMaxOp = LHS;<br>
> - ThirdOp = C;<br>
> - } else if (C == A || C == B) {<br>
> - // min(min(a, b), min(b, d)) --> min(min(a, b), d)<br>
> - // min(min(a, b), min(c, b)) --> min(min(a, b), d)<br>
> - MinMaxOp = LHS;<br>
> - ThirdOp = D;<br>
> - }<br>
> - }<br>
> -<br>
> - Module *Mod = II->getModule();<br>
> - Function *MinMax = Intrinsic::getDeclaration(Mod, MinMaxID, II->getType());<br>
> - return CallInst::Create(MinMax, { MinMaxOp, ThirdOp });<br>
> -}<br>
> -<br>
> /// CallInst simplification. This mostly only handles folding of intrinsic<br>
> /// instructions. For normal calls, it allows visitCallBase to do the heavy<br>
> /// lifting.<br>
> @@ -1110,9 +1056,6 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {<br>
> if (Instruction *R = FoldOpIntoSelect(*II, Sel))<br>
> return R;<br>
><br>
> - if (Instruction *NewMinMax = factorizeMinMaxTree(II))<br>
> - return NewMinMax;<br>
> -<br>
> break;<br>
> }<br>
> case Intrinsic::bswap: {<br>
><br>
> diff --git a/llvm/test/Transforms/InstCombine/minmax-intrinsics.ll b/llvm/test/Transforms/InstCombine/minmax-intrinsics.ll<br>
> index 8693526f198c..54e133ded3c9 100644<br>
> --- a/llvm/test/Transforms/InstCombine/minmax-intrinsics.ll<br>
> +++ b/llvm/test/Transforms/InstCombine/minmax-intrinsics.ll<br>
> @@ -907,8 +907,9 @@ define <3 x i1> @umin_ne_zero2(<3 x i8> %a, <3 x i8> %b) {<br>
><br>
> define i8 @smax(i8 %x, i8 %y, i8 %z) {<br>
> ; CHECK-LABEL: @smax(<br>
> -; CHECK-NEXT: [[M2:%.*]] = call i8 @llvm.smax.i8(i8 [[X:%.*]], i8 [[Z:%.*]])<br>
> -; CHECK-NEXT: [[M3:%.*]] = call i8 @llvm.smax.i8(i8 [[M2]], i8 [[Y:%.*]])<br>
> +; CHECK-NEXT: [[M1:%.*]] = call i8 @llvm.smax.i8(i8 [[X:%.*]], i8 [[Y:%.*]])<br>
> +; CHECK-NEXT: [[M2:%.*]] = call i8 @llvm.smax.i8(i8 [[X]], i8 [[Z:%.*]])<br>
> +; CHECK-NEXT: [[M3:%.*]] = call i8 @llvm.smax.i8(i8 [[M1]], i8 [[M2]])<br>
> ; CHECK-NEXT: ret i8 [[M3]]<br>
> ;<br>
> %m1 = call i8 @llvm.smax.i8(i8 %x, i8 %y)<br>
> @@ -919,8 +920,9 @@ define i8 @smax(i8 %x, i8 %y, i8 %z) {<br>
><br>
> define <3 x i8> @smin(<3 x i8> %x, <3 x i8> %y, <3 x i8> %z) {<br>
> ; CHECK-LABEL: @smin(<br>
> -; CHECK-NEXT: [[M2:%.*]] = call <3 x i8> @llvm.smin.v3i8(<3 x i8> [[X:%.*]], <3 x i8> [[Z:%.*]])<br>
> -; CHECK-NEXT: [[M3:%.*]] = call <3 x i8> @llvm.smin.v3i8(<3 x i8> [[M2]], <3 x i8> [[Y:%.*]])<br>
> +; CHECK-NEXT: [[M1:%.*]] = call <3 x i8> @llvm.smin.v3i8(<3 x i8> [[Y:%.*]], <3 x i8> [[X:%.*]])<br>
> +; CHECK-NEXT: [[M2:%.*]] = call <3 x i8> @llvm.smin.v3i8(<3 x i8> [[X]], <3 x i8> [[Z:%.*]])<br>
> +; CHECK-NEXT: [[M3:%.*]] = call <3 x i8> @llvm.smin.v3i8(<3 x i8> [[M1]], <3 x i8> [[M2]])<br>
> ; CHECK-NEXT: ret <3 x i8> [[M3]]<br>
> ;<br>
> %m1 = call <3 x i8> @llvm.smin.v3i8(<3 x i8> %y, <3 x i8> %x)<br>
> @@ -933,7 +935,8 @@ define i8 @umax(i8 %x, i8 %y, i8 %z) {<br>
> ; CHECK-LABEL: @umax(<br>
> ; CHECK-NEXT: [[M1:%.*]] = call i8 @llvm.umax.i8(i8 [[X:%.*]], i8 [[Y:%.*]])<br>
> ; CHECK-NEXT: call void @use(i8 [[M1]])<br>
> -; CHECK-NEXT: [[M3:%.*]] = call i8 @llvm.umax.i8(i8 [[M1]], i8 [[Z:%.*]])<br>
> +; CHECK-NEXT: [[M2:%.*]] = call i8 @llvm.umax.i8(i8 [[Z:%.*]], i8 [[X]])<br>
> +; CHECK-NEXT: [[M3:%.*]] = call i8 @llvm.umax.i8(i8 [[M1]], i8 [[M2]])<br>
> ; CHECK-NEXT: ret i8 [[M3]]<br>
> ;<br>
> %m1 = call i8 @llvm.umax.i8(i8 %x, i8 %y)<br>
> @@ -945,9 +948,10 @@ define i8 @umax(i8 %x, i8 %y, i8 %z) {<br>
><br>
> define i8 @umin(i8 %x, i8 %y, i8 %z) {<br>
> ; CHECK-LABEL: @umin(<br>
> -; CHECK-NEXT: [[M2:%.*]] = call i8 @llvm.umin.i8(i8 [[Z:%.*]], i8 [[X:%.*]])<br>
> +; CHECK-NEXT: [[M1:%.*]] = call i8 @llvm.umin.i8(i8 [[Y:%.*]], i8 [[X:%.*]])<br>
> +; CHECK-NEXT: [[M2:%.*]] = call i8 @llvm.umin.i8(i8 [[Z:%.*]], i8 [[X]])<br>
> ; CHECK-NEXT: call void @use(i8 [[M2]])<br>
> -; CHECK-NEXT: [[M3:%.*]] = call i8 @llvm.umin.i8(i8 [[M2]], i8 [[Y:%.*]])<br>
> +; CHECK-NEXT: [[M3:%.*]] = call i8 @llvm.umin.i8(i8 [[M1]], i8 [[M2]])<br>
> ; CHECK-NEXT: ret i8 [[M3]]<br>
> ;<br>
> %m1 = call i8 @llvm.umin.i8(i8 %y, i8 %x)<br>
><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>