<div dir="ltr"><div>Yes - this was the quick and easy fix for the motivating example in PR47430 (forgot to note that in the commit message), but we can go further. <br></div><div>I noticed the same/similar missing multiply-by-negative-power-of-2 fold here:<br></div><div><a href="https://bugs.llvm.org/show_bug.cgi?id=47430#c3">https://bugs.llvm.org/show_bug.cgi?id=47430#c3</a></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 24, 2020 at 2:34 PM Roman Lebedev <<a href="mailto:lebedev.ri@gmail.com">lebedev.ri@gmail.com</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">On Tue, Nov 24, 2020 at 9:56 PM Sanjay Patel 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: Sanjay Patel<br>
> Date: 2020-11-24T13:56:30-05:00<br>
> New Revision: 678b9c5dde0d119e91f5f905c3d9101cf8c514f9<br>
><br>
> URL: <a href="https://github.com/llvm/llvm-project/commit/678b9c5dde0d119e91f5f905c3d9101cf8c514f9" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/678b9c5dde0d119e91f5f905c3d9101cf8c514f9</a><br>
> DIFF: <a href="https://github.com/llvm/llvm-project/commit/678b9c5dde0d119e91f5f905c3d9101cf8c514f9.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/678b9c5dde0d119e91f5f905c3d9101cf8c514f9.diff</a><br>
><br>
> LOG: [InstCombine] try difference-of-shifts factorization before negator<br>
><br>
> We need to preserve wrapping flags to allow better folds.<br>
> The cases with geps may be non-intuitive, but that appears to agree with Alive2:<br>
> <a href="https://alive2.llvm.org/ce/z/JQcqw7" rel="noreferrer" target="_blank">https://alive2.llvm.org/ce/z/JQcqw7</a><br>
> We create 'nsw' ops independent from the original wrapping on the sub.<br>
><br>
> Added:<br>
><br>
><br>
> Modified:<br>
> llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp<br>
> llvm/test/Transforms/InstCombine/sub-gep.ll<br>
> llvm/test/Transforms/InstCombine/sub.ll<br>
><br>
> Removed:<br>
><br>
><br>
><br>
> ################################################################################<br>
> diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp<br>
> index 9a6a790aefaf..bf356d0de94b 100644<br>
> --- a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp<br>
> +++ b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp<br>
> @@ -1722,6 +1722,10 @@ Instruction *InstCombinerImpl::visitSub(BinaryOperator &I) {<br>
> return Res;<br>
> }<br>
><br>
> + // Try this before Negator to preserve NSW flag.<br>
> + if (Instruction *R = factorizeMathWithShlOps(I, Builder))<br>
> + return R;<br>
> +<br>
> if (Constant *C = dyn_cast<Constant>(Op0)) {<br>
> Value *X;<br>
> Constant *C2;<br>
> @@ -1770,9 +1774,6 @@ Instruction *InstCombinerImpl::visitSub(BinaryOperator &I) {<br>
> if (Value *V = SimplifyUsingDistributiveLaws(I))<br>
> return replaceInstUsesWith(I, V);<br>
><br>
> - if (Instruction *R = factorizeMathWithShlOps(I, Builder))<br>
> - return R;<br>
> -<br>
> if (I.getType()->isIntOrIntVectorTy(1))<br>
> return BinaryOperator::CreateXor(Op0, Op1);<br>
><br>
><br>
> diff --git a/llvm/test/Transforms/InstCombine/sub-gep.ll b/llvm/test/Transforms/InstCombine/sub-gep.ll<br>
> index d6ace8275c10..bc18608da7a5 100644<br>
> --- a/llvm/test/Transforms/InstCombine/sub-gep.ll<br>
> +++ b/llvm/test/Transforms/InstCombine/sub-gep.ll<br>
> @@ -125,9 +125,8 @@ define i64 @test_inbounds2_nuw_swapped([0 x i32]* %base, i64 %idx) {<br>
><br>
> define i64 @test_inbounds_two_gep([0 x i32]* %base, i64 %idx, i64 %idx2) {<br>
<br>
> ; CHECK-LABEL: @test_inbounds_two_gep(<br>
> -; CHECK-NEXT: [[P2_IDX:%.*]] = shl nsw i64 [[IDX2:%.*]], 2<br>
> -; CHECK-NEXT: [[P1_IDX_NEG:%.*]] = mul i64 [[IDX:%.*]], -4<br>
> -; CHECK-NEXT: [[GEPDIFF:%.*]] = add i64 [[P1_IDX_NEG]], [[P2_IDX]]<br>
> +; CHECK-NEXT: [[TMP1:%.*]] = sub nsw i64 [[IDX2:%.*]], [[IDX:%.*]]<br>
> +; CHECK-NEXT: [[GEPDIFF:%.*]] = shl nsw i64 [[TMP1]], 2<br>
> ; CHECK-NEXT: ret i64 [[GEPDIFF]]<br>
FWIW modulo the no-wrap flags we could/should be getting this already:<br>
<a href="https://rise4fun.com/Alive/ER4" rel="noreferrer" target="_blank">https://rise4fun.com/Alive/ER4</a><br>
<br>
> ;<br>
> %p1 = getelementptr inbounds [0 x i32], [0 x i32]* %base, i64 0, i64 %idx<br>
> @@ -140,9 +139,8 @@ define i64 @test_inbounds_two_gep([0 x i32]* %base, i64 %idx, i64 %idx2) {<br>
><br>
> define i64 @test_inbounds_nsw_two_gep([0 x i32]* %base, i64 %idx, i64 %idx2) {<br>
> ; CHECK-LABEL: @test_inbounds_nsw_two_gep(<br>
> -; CHECK-NEXT: [[P2_IDX:%.*]] = shl nsw i64 [[IDX2:%.*]], 2<br>
> -; CHECK-NEXT: [[P1_IDX_NEG:%.*]] = mul i64 [[IDX:%.*]], -4<br>
> -; CHECK-NEXT: [[GEPDIFF:%.*]] = add i64 [[P1_IDX_NEG]], [[P2_IDX]]<br>
> +; CHECK-NEXT: [[TMP1:%.*]] = sub nsw i64 [[IDX2:%.*]], [[IDX:%.*]]<br>
> +; CHECK-NEXT: [[GEPDIFF:%.*]] = shl nsw i64 [[TMP1]], 2<br>
> ; CHECK-NEXT: ret i64 [[GEPDIFF]]<br>
> ;<br>
> %p1 = getelementptr inbounds [0 x i32], [0 x i32]* %base, i64 0, i64 %idx<br>
> @@ -155,9 +153,8 @@ define i64 @test_inbounds_nsw_two_gep([0 x i32]* %base, i64 %idx, i64 %idx2) {<br>
><br>
> define i64 @test_inbounds_nuw_two_gep([0 x i32]* %base, i64 %idx, i64 %idx2) {<br>
> ; CHECK-LABEL: @test_inbounds_nuw_two_gep(<br>
> -; CHECK-NEXT: [[P2_IDX:%.*]] = shl nsw i64 [[IDX2:%.*]], 2<br>
> -; CHECK-NEXT: [[P1_IDX_NEG:%.*]] = mul i64 [[IDX:%.*]], -4<br>
> -; CHECK-NEXT: [[GEPDIFF:%.*]] = add i64 [[P1_IDX_NEG]], [[P2_IDX]]<br>
> +; CHECK-NEXT: [[TMP1:%.*]] = sub nsw i64 [[IDX2:%.*]], [[IDX:%.*]]<br>
> +; CHECK-NEXT: [[GEPDIFF:%.*]] = shl nsw i64 [[TMP1]], 2<br>
> ; CHECK-NEXT: ret i64 [[GEPDIFF]]<br>
> ;<br>
> %p1 = getelementptr inbounds [0 x i32], [0 x i32]* %base, i64 0, i64 %idx<br>
><br>
> diff --git a/llvm/test/Transforms/InstCombine/sub.ll b/llvm/test/Transforms/InstCombine/sub.ll<br>
> index f325f8d09cf9..066085fc2535 100644<br>
> --- a/llvm/test/Transforms/InstCombine/sub.ll<br>
> +++ b/llvm/test/Transforms/InstCombine/sub.ll<br>
> @@ -1505,9 +1505,8 @@ define <2 x i8> @sub_mask_lowbits_splat_extra_use(<2 x i8> %x, <2 x i8>* %p) {<br>
><br>
> define i16 @sub_nsw_mul_nsw(i16 %x, i16 %y) {<br>
> ; CHECK-LABEL: @sub_nsw_mul_nsw(<br>
> -; CHECK-NEXT: [[X8:%.*]] = shl nsw i16 [[X:%.*]], 3<br>
> -; CHECK-NEXT: [[Y8_NEG:%.*]] = mul i16 [[Y:%.*]], -8<br>
> -; CHECK-NEXT: [[R:%.*]] = add i16 [[Y8_NEG]], [[X8]]<br>
> +; CHECK-NEXT: [[TMP1:%.*]] = sub nsw i16 [[X:%.*]], [[Y:%.*]]<br>
> +; CHECK-NEXT: [[R:%.*]] = shl nsw i16 [[TMP1]], 3<br>
> ; CHECK-NEXT: ret i16 [[R]]<br>
> ;<br>
> %x8 = mul nsw i16 %x, 8<br>
> @@ -1518,9 +1517,8 @@ define i16 @sub_nsw_mul_nsw(i16 %x, i16 %y) {<br>
><br>
> define i16 @sub_nuw_mul_nsw(i16 %x, i16 %y) {<br>
> ; CHECK-LABEL: @sub_nuw_mul_nsw(<br>
> -; CHECK-NEXT: [[X8:%.*]] = shl nsw i16 [[X:%.*]], 2<br>
> -; CHECK-NEXT: [[Y8_NEG:%.*]] = mul i16 [[Y:%.*]], -4<br>
> -; CHECK-NEXT: [[R:%.*]] = add i16 [[Y8_NEG]], [[X8]]<br>
> +; CHECK-NEXT: [[TMP1:%.*]] = sub i16 [[X:%.*]], [[Y:%.*]]<br>
> +; CHECK-NEXT: [[R:%.*]] = shl i16 [[TMP1]], 2<br>
> ; CHECK-NEXT: ret i16 [[R]]<br>
> ;<br>
> %x8 = mul nsw i16 %x, 4<br>
> @@ -1531,9 +1529,8 @@ define i16 @sub_nuw_mul_nsw(i16 %x, i16 %y) {<br>
><br>
> define i16 @sub_mul_nsw(i16 %x, i16 %y) {<br>
> ; CHECK-LABEL: @sub_mul_nsw(<br>
> -; CHECK-NEXT: [[X8:%.*]] = shl nsw i16 [[X:%.*]], 4<br>
> -; CHECK-NEXT: [[Y8_NEG:%.*]] = mul i16 [[Y:%.*]], -16<br>
> -; CHECK-NEXT: [[R:%.*]] = add i16 [[Y8_NEG]], [[X8]]<br>
> +; CHECK-NEXT: [[TMP1:%.*]] = sub i16 [[X:%.*]], [[Y:%.*]]<br>
> +; CHECK-NEXT: [[R:%.*]] = shl i16 [[TMP1]], 4<br>
> ; CHECK-NEXT: ret i16 [[R]]<br>
> ;<br>
> %x8 = mul nsw i16 %x, 16<br>
> @@ -1544,9 +1541,8 @@ define i16 @sub_mul_nsw(i16 %x, i16 %y) {<br>
><br>
> define i16 @sub_nsw_mul_nuw(i16 %x, i16 %y) {<br>
> ; CHECK-LABEL: @sub_nsw_mul_nuw(<br>
> -; CHECK-NEXT: [[X8:%.*]] = shl nuw i16 [[X:%.*]], 3<br>
> -; CHECK-NEXT: [[Y8_NEG:%.*]] = mul i16 [[Y:%.*]], -8<br>
> -; CHECK-NEXT: [[R:%.*]] = add i16 [[Y8_NEG]], [[X8]]<br>
> +; CHECK-NEXT: [[TMP1:%.*]] = sub i16 [[X:%.*]], [[Y:%.*]]<br>
> +; CHECK-NEXT: [[R:%.*]] = shl i16 [[TMP1]], 3<br>
> ; CHECK-NEXT: ret i16 [[R]]<br>
> ;<br>
> %x8 = mul nuw i16 %x, 8<br>
> @@ -1557,9 +1553,8 @@ define i16 @sub_nsw_mul_nuw(i16 %x, i16 %y) {<br>
><br>
> define i16 @sub_nuw_mul_nuw(i16 %x, i16 %y) {<br>
> ; CHECK-LABEL: @sub_nuw_mul_nuw(<br>
> -; CHECK-NEXT: [[X8:%.*]] = shl nuw i16 [[X:%.*]], 4<br>
> -; CHECK-NEXT: [[Y8_NEG:%.*]] = mul i16 [[Y:%.*]], -16<br>
> -; CHECK-NEXT: [[R:%.*]] = add i16 [[Y8_NEG]], [[X8]]<br>
> +; CHECK-NEXT: [[TMP1:%.*]] = sub nuw i16 [[X:%.*]], [[Y:%.*]]<br>
> +; CHECK-NEXT: [[R:%.*]] = shl nuw i16 [[TMP1]], 4<br>
> ; CHECK-NEXT: ret i16 [[R]]<br>
> ;<br>
> %x8 = mul nuw i16 %x, 16<br>
> @@ -1570,9 +1565,8 @@ define i16 @sub_nuw_mul_nuw(i16 %x, i16 %y) {<br>
><br>
> define i16 @sub_mul_nuw(i16 %x, i16 %y) {<br>
> ; CHECK-LABEL: @sub_mul_nuw(<br>
> -; CHECK-NEXT: [[X8:%.*]] = shl nuw i16 [[X:%.*]], 5<br>
> -; CHECK-NEXT: [[Y8_NEG:%.*]] = mul i16 [[Y:%.*]], -32<br>
> -; CHECK-NEXT: [[R:%.*]] = add i16 [[Y8_NEG]], [[X8]]<br>
> +; CHECK-NEXT: [[TMP1:%.*]] = sub i16 [[X:%.*]], [[Y:%.*]]<br>
> +; CHECK-NEXT: [[R:%.*]] = shl i16 [[TMP1]], 5<br>
> ; CHECK-NEXT: ret i16 [[R]]<br>
> ;<br>
> %x8 = mul nuw i16 %x, 32<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>
</blockquote></div>