<div dir="ltr"><div>Our rules for tie-breaking for canonical form are created on-the-fly / self-referential...so whatever we put more effort into becomes canonical. :) <br></div><div><br></div><div>In this case, analysis for shifts is better in general than multiplies, so we'll need to look out for regressions if we are now creating more multiplies.</div><div><br></div><div>For example, we seem to be missing these icmp folds (but some constants are handled, so we probably have over-specific code somewhere):</div><div><a href="https://rise4fun.com/Alive/sdM9">https://rise4fun.com/Alive/sdM9</a></div><div>I can look into that one.<br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Aug 5, 2020 at 7:16 AM 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 Wed, Aug 5, 2020 at 11:46 AM Nikita Popov <<a href="mailto:nikita.ppv@gmail.com" target="_blank">nikita.ppv@gmail.com</a>> wrote:<br>
> This looks a bit dubious to me.<br>
Please can you be more specific?<br>
mul isn't obviously much worse than shl.<br>
<br>
FWIW i saw this in real IR, i expected it to be already handled<br>
by Negator, but due to instruction visitation order the mul that i saw<br>
in the IR got converted into a shl before we've encountered negation.<br>
<br>
> Shouldn't we prefer a shift over a multiply?<br>
Yep, we should, that is why i try to sink the negation first,<br>
and only if that fails i convert shl -> mul.<br>
<br>
> Especially in cases where we don't actually eliminate a sub...<br>
We didn't increase instruction count and we changed non-commutable<br>
instruction (sub) into a commutable instruction (add),<br>
so as far as instcombine's "cost-model" is concerned this is fine.<br>
<br>
> Nikita<br>
<br>
Roman<br>
<br>
> On Wed, Aug 5, 2020 at 2:13 AM Roman Lebedev via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>><br>
>> Author: Roman Lebedev<br>
>> Date: 2020-08-05T03:13:14+03:00<br>
>> New Revision: 8aeb2fe13a4100b4c2e78d6ef75119304100cb1f<br>
>><br>
>> URL: <a href="https://github.com/llvm/llvm-project/commit/8aeb2fe13a4100b4c2e78d6ef75119304100cb1f" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/8aeb2fe13a4100b4c2e78d6ef75119304100cb1f</a><br>
>> DIFF: <a href="https://github.com/llvm/llvm-project/commit/8aeb2fe13a4100b4c2e78d6ef75119304100cb1f.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/8aeb2fe13a4100b4c2e78d6ef75119304100cb1f.diff</a><br>
>><br>
>> LOG: [InstCombine] Negator: -(X << C) --> X * (-1 << C)<br>
>><br>
>> This shows some regressions in tests, but they are all around GEP's,<br>
>> so i'm not really sure how important those are.<br>
>><br>
>> <a href="https://rise4fun.com/Alive/1Gn" rel="noreferrer" target="_blank">https://rise4fun.com/Alive/1Gn</a><br>
>><br>
>> Added:<br>
>><br>
>><br>
>> Modified:<br>
>> llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp<br>
>> llvm/test/Transforms/InstCombine/icmp.ll<br>
>> llvm/test/Transforms/InstCombine/sub-gep.ll<br>
>> llvm/test/Transforms/InstCombine/sub-of-negatible.ll<br>
>> llvm/test/Transforms/InstCombine/sub.ll<br>
>><br>
>> Removed:<br>
>><br>
>><br>
>><br>
>> ################################################################################<br>
>> diff --git a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp<br>
>> index 1c7f00b0edee..b684016b6a29 100644<br>
>> --- a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp<br>
>> +++ b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp<br>
>> @@ -324,10 +324,16 @@ LLVM_NODISCARD Value *Negator::visitImpl(Value *V, unsigned Depth) {<br>
>> }<br>
>> case Instruction::Shl: {<br>
>> // `shl` is negatible if the first operand is negatible.<br>
>> - Value *NegOp0 = negate(I->getOperand(0), Depth + 1);<br>
>> - if (!NegOp0) // Early return.<br>
>> + if (Value *NegOp0 = negate(I->getOperand(0), Depth + 1))<br>
>> + return Builder.CreateShl(NegOp0, I->getOperand(1), I->getName() + ".neg");<br>
>> + // Otherwise, `shl %x, C` can be interpreted as `mul %x, 1<<C`.<br>
>> + auto *Op1C = dyn_cast<Constant>(I->getOperand(1));<br>
>> + if (!Op1C) // Early return.<br>
>> return nullptr;<br>
>> - return Builder.CreateShl(NegOp0, I->getOperand(1), I->getName() + ".neg");<br>
>> + return Builder.CreateMul(<br>
>> + I->getOperand(0),<br>
>> + ConstantExpr::getShl(Constant::getAllOnesValue(Op1C->getType()), Op1C),<br>
>> + I->getName() + ".neg");<br>
>> }<br>
>> case Instruction::Or:<br>
>> if (!haveNoCommonBitsSet(I->getOperand(0), I->getOperand(1), DL, &AC, I,<br>
>><br>
>> diff --git a/llvm/test/Transforms/InstCombine/icmp.ll b/llvm/test/Transforms/InstCombine/icmp.ll<br>
>> index e3050aa1bac2..7addc85c4c8b 100644<br>
>> --- a/llvm/test/Transforms/InstCombine/icmp.ll<br>
>> +++ b/llvm/test/Transforms/InstCombine/icmp.ll<br>
>> @@ -512,7 +512,8 @@ define i1 @test24(i64 %i) {<br>
>> ; unsigned overflow does not happen during offset computation<br>
>> define i1 @test24_neg_offs(i32* %p, i64 %offs) {<br>
>> ; CHECK-LABEL: @test24_neg_offs(<br>
>> -; CHECK-NEXT: [[CMP:%.*]] = icmp eq i64 [[OFFS:%.*]], -2<br>
>> +; CHECK-NEXT: [[P1_IDX_NEG:%.*]] = mul i64 [[OFFS:%.*]], -4<br>
>> +; CHECK-NEXT: [[CMP:%.*]] = icmp eq i64 [[P1_IDX_NEG]], 8<br>
>> ; CHECK-NEXT: ret i1 [[CMP]]<br>
>> ;<br>
>> %p1 = getelementptr inbounds i32, i32* %p, i64 %offs<br>
>><br>
>> diff --git a/llvm/test/Transforms/InstCombine/sub-gep.ll b/llvm/test/Transforms/InstCombine/sub-gep.ll<br>
>> index 51eb994bf345..cf9604223f6c 100644<br>
>> --- a/llvm/test/Transforms/InstCombine/sub-gep.ll<br>
>> +++ b/llvm/test/Transforms/InstCombine/sub-gep.ll<br>
>> @@ -58,9 +58,8 @@ define i32 @test_inbounds_nuw_trunc([0 x i32]* %base, i64 %idx) {<br>
>><br>
>> define i64 @test_inbounds_nuw_swapped([0 x i32]* %base, i64 %idx) {<br>
>> ; CHECK-LABEL: @test_inbounds_nuw_swapped(<br>
>> -; CHECK-NEXT: [[P2_IDX:%.*]] = shl nsw i64 [[IDX:%.*]], 2<br>
>> -; CHECK-NEXT: [[DIFF_NEG:%.*]] = sub i64 0, [[P2_IDX]]<br>
>> -; CHECK-NEXT: ret i64 [[DIFF_NEG]]<br>
>> +; CHECK-NEXT: [[P2_IDX_NEG:%.*]] = mul i64 [[IDX:%.*]], -4<br>
>> +; CHECK-NEXT: ret i64 [[P2_IDX_NEG]]<br>
>> ;<br>
>> %p1 = getelementptr inbounds [0 x i32], [0 x i32]* %base, i64 0, i64 0<br>
>> %p2 = getelementptr inbounds [0 x i32], [0 x i32]* %base, i64 0, i64 %idx<br>
>> @@ -73,8 +72,9 @@ define i64 @test_inbounds_nuw_swapped([0 x i32]* %base, i64 %idx) {<br>
>> ; The sub and shl here could be nuw, but this is harder to handle.<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: [[P1_IDX1_NEG:%.*]] = sub i64 [[IDX2:%.*]], [[IDX:%.*]]<br>
>> -; CHECK-NEXT: [[DOTNEG:%.*]] = shl i64 [[P1_IDX1_NEG]], 2<br>
>> +; CHECK-NEXT: [[P1_IDX_NEG:%.*]] = mul i64 [[IDX:%.*]], -4<br>
>> +; CHECK-NEXT: [[P2_IDX_NEG_NEG:%.*]] = shl i64 [[IDX2:%.*]], 2<br>
>> +; CHECK-NEXT: [[DOTNEG:%.*]] = add i64 [[P2_IDX_NEG_NEG]], [[P1_IDX_NEG]]<br>
>> ; CHECK-NEXT: ret i64 [[DOTNEG]]<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-of-negatible.ll b/llvm/test/Transforms/InstCombine/sub-of-negatible.ll<br>
>> index caa6e25ccf69..4a3c56337c22 100644<br>
>> --- a/llvm/test/Transforms/InstCombine/sub-of-negatible.ll<br>
>> +++ b/llvm/test/Transforms/InstCombine/sub-of-negatible.ll<br>
>> @@ -1076,8 +1076,8 @@ define i8 @negate_left_shift_by_constant(i8 %x, i8 %y, i8 %z, i8 %k) {<br>
>> ; CHECK-LABEL: @negate_left_shift_by_constant(<br>
>> ; CHECK-NEXT: [[T0:%.*]] = sub i8 [[K:%.*]], [[Z:%.*]]<br>
>> ; CHECK-NEXT: call void @use8(i8 [[T0]])<br>
>> -; CHECK-NEXT: [[T1:%.*]] = shl i8 [[T0]], 4<br>
>> -; CHECK-NEXT: [[T2:%.*]] = sub i8 [[X:%.*]], [[T1]]<br>
>> +; CHECK-NEXT: [[T1_NEG:%.*]] = mul i8 [[T0]], -16<br>
>> +; CHECK-NEXT: [[T2:%.*]] = add i8 [[T1_NEG]], [[X:%.*]]<br>
>> ; CHECK-NEXT: ret i8 [[T2]]<br>
>> ;<br>
>> %t0 = sub i8 %k, %z<br>
>><br>
>> diff --git a/llvm/test/Transforms/InstCombine/sub.ll b/llvm/test/Transforms/InstCombine/sub.ll<br>
>> index 3bc8b67649fc..4116a79d66d9 100644<br>
>> --- a/llvm/test/Transforms/InstCombine/sub.ll<br>
>> +++ b/llvm/test/Transforms/InstCombine/sub.ll<br>
>> @@ -506,8 +506,8 @@ define i64 @test24b(i8* %P, i64 %A){<br>
>><br>
>> define i64 @test25(i8* %P, i64 %A){<br>
>> ; CHECK-LABEL: @test25(<br>
>> -; CHECK-NEXT: [[B_IDX:%.*]] = shl nsw i64 [[A:%.*]], 1<br>
>> -; CHECK-NEXT: [[DOTNEG:%.*]] = add i64 [[B_IDX]], -84<br>
>> +; CHECK-NEXT: [[B_IDX_NEG_NEG:%.*]] = shl i64 [[A:%.*]], 1<br>
>> +; CHECK-NEXT: [[DOTNEG:%.*]] = add i64 [[B_IDX_NEG_NEG]], -84<br>
>> ; CHECK-NEXT: ret i64 [[DOTNEG]]<br>
>> ;<br>
>> %B = getelementptr inbounds [42 x i16], [42 x i16]* @Arr, i64 0, i64 %A<br>
>> @@ -521,8 +521,8 @@ define i64 @test25(i8* %P, i64 %A){<br>
>> define i16 @test25_as1(i8 addrspace(1)* %P, i64 %A) {<br>
>> ; CHECK-LABEL: @test25_as1(<br>
>> ; CHECK-NEXT: [[TMP1:%.*]] = trunc i64 [[A:%.*]] to i16<br>
>> -; CHECK-NEXT: [[B_IDX:%.*]] = shl nsw i16 [[TMP1]], 1<br>
>> -; CHECK-NEXT: [[DOTNEG:%.*]] = add i16 [[B_IDX]], -84<br>
>> +; CHECK-NEXT: [[B_IDX_NEG_NEG:%.*]] = shl i16 [[TMP1]], 1<br>
>> +; CHECK-NEXT: [[DOTNEG:%.*]] = add i16 [[B_IDX_NEG_NEG]], -84<br>
>> ; CHECK-NEXT: ret i16 [[DOTNEG]]<br>
>> ;<br>
>> %B = getelementptr inbounds [42 x i16], [42 x i16] addrspace(1)* @Arr_as1, i64 0, i64 %A<br>
>> @@ -557,9 +557,8 @@ define i64 @test_neg_shl_sub_extra_use1(i64 %a, i64 %b, i64* %p) {<br>
>> ; CHECK-LABEL: @test_neg_shl_sub_extra_use1(<br>
>> ; CHECK-NEXT: [[SUB:%.*]] = sub i64 [[A:%.*]], [[B:%.*]]<br>
>> ; CHECK-NEXT: store i64 [[SUB]], i64* [[P:%.*]], align 8<br>
>> -; CHECK-NEXT: [[MUL:%.*]] = shl i64 [[SUB]], 2<br>
>> -; CHECK-NEXT: [[NEG:%.*]] = sub i64 0, [[MUL]]<br>
>> -; CHECK-NEXT: ret i64 [[NEG]]<br>
>> +; CHECK-NEXT: [[MUL_NEG:%.*]] = mul i64 [[SUB]], -4<br>
>> +; CHECK-NEXT: ret i64 [[MUL_NEG]]<br>
>> ;<br>
>> %sub = sub i64 %a, %b<br>
>> store i64 %sub, i64* %p<br>
>> @@ -840,9 +839,10 @@ define i64 @test29(i8* %foo, i64 %i, i64 %j) {<br>
>><br>
>> define i64 @test30(i8* %foo, i64 %i, i64 %j) {<br>
>> ; CHECK-LABEL: @test30(<br>
>> -; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i64 [[I:%.*]], 2<br>
>> -; CHECK-NEXT: [[DOTNEG:%.*]] = sub i64 [[GEP1_IDX]], [[J:%.*]]<br>
>> -; CHECK-NEXT: ret i64 [[DOTNEG]]<br>
>> +; CHECK-NEXT: [[GEP1_IDX_NEG:%.*]] = mul i64 [[I:%.*]], -4<br>
>> +; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[GEP1_IDX_NEG]], [[J:%.*]]<br>
>> +; CHECK-NEXT: [[DIFF_NEG:%.*]] = sub i64 0, [[TMP1]]<br>
>> +; CHECK-NEXT: ret i64 [[DIFF_NEG]]<br>
>> ;<br>
>> %bit = bitcast i8* %foo to i32*<br>
>> %gep1 = getelementptr inbounds i32, i32* %bit, i64 %i<br>
>> @@ -855,9 +855,10 @@ define i64 @test30(i8* %foo, i64 %i, i64 %j) {<br>
>><br>
>> define i16 @test30_as1(i8 addrspace(1)* %foo, i16 %i, i16 %j) {<br>
>> ; CHECK-LABEL: @test30_as1(<br>
>> -; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i16 [[I:%.*]], 2<br>
>> -; CHECK-NEXT: [[DOTNEG:%.*]] = sub i16 [[GEP1_IDX]], [[J:%.*]]<br>
>> -; CHECK-NEXT: ret i16 [[DOTNEG]]<br>
>> +; CHECK-NEXT: [[GEP1_IDX_NEG:%.*]] = mul i16 [[I:%.*]], -4<br>
>> +; CHECK-NEXT: [[TMP1:%.*]] = add i16 [[GEP1_IDX_NEG]], [[J:%.*]]<br>
>> +; CHECK-NEXT: [[DIFF_NEG:%.*]] = sub i16 0, [[TMP1]]<br>
>> +; CHECK-NEXT: ret i16 [[DIFF_NEG]]<br>
>> ;<br>
>> %bit = bitcast i8 addrspace(1)* %foo to i32 addrspace(1)*<br>
>> %gep1 = getelementptr inbounds i32, i32 addrspace(1)* %bit, i16 %i<br>
>> @@ -1310,8 +1311,8 @@ define i64 @test61([100 x [100 x i8]]* %foo, i64 %i, i64 %j) {<br>
>><br>
>> define i32 @test62(i32 %A) {<br>
>> ; CHECK-LABEL: @test62(<br>
>> -; CHECK-NEXT: [[B:%.*]] = shl i32 [[A:%.*]], 1<br>
>> -; CHECK-NEXT: [[C:%.*]] = sub i32 2, [[B]]<br>
>> +; CHECK-NEXT: [[B_NEG:%.*]] = mul i32 [[A:%.*]], -2<br>
>> +; CHECK-NEXT: [[C:%.*]] = add i32 [[B_NEG]], 2<br>
>> ; CHECK-NEXT: ret i32 [[C]]<br>
>> ;<br>
>> %B = sub i32 1, %A<br>
>> @@ -1321,8 +1322,8 @@ define i32 @test62(i32 %A) {<br>
>><br>
>> define <2 x i32> @test62vec(<2 x i32> %A) {<br>
>> ; CHECK-LABEL: @test62vec(<br>
>> -; CHECK-NEXT: [[B:%.*]] = shl <2 x i32> [[A:%.*]], <i32 1, i32 1><br>
>> -; CHECK-NEXT: [[C:%.*]] = sub <2 x i32> <i32 2, i32 2>, [[B]]<br>
>> +; CHECK-NEXT: [[B_NEG:%.*]] = mul <2 x i32> [[A:%.*]], <i32 -2, i32 -2><br>
>> +; CHECK-NEXT: [[C:%.*]] = add <2 x i32> [[B_NEG]], <i32 2, i32 2><br>
>> ; CHECK-NEXT: ret <2 x i32> [[C]]<br>
>> ;<br>
>> %B = sub <2 x i32> <i32 1, i32 1>, %A<br>
>> @@ -1332,8 +1333,8 @@ define <2 x i32> @test62vec(<2 x i32> %A) {<br>
>><br>
>> define i32 @test63(i32 %A) {<br>
>> ; CHECK-LABEL: @test63(<br>
>> -; CHECK-NEXT: [[B:%.*]] = shl i32 [[A:%.*]], 1<br>
>> -; CHECK-NEXT: ret i32 [[B]]<br>
>> +; CHECK-NEXT: [[B_NEG_NEG:%.*]] = shl i32 [[A:%.*]], 1<br>
>> +; CHECK-NEXT: ret i32 [[B_NEG_NEG]]<br>
>> ;<br>
>> %B = sub i32 1, %A<br>
>> %C = shl i32 %B, 1<br>
>> @@ -1343,8 +1344,8 @@ define i32 @test63(i32 %A) {<br>
>><br>
>> define <2 x i32> @test63vec(<2 x i32> %A) {<br>
>> ; CHECK-LABEL: @test63vec(<br>
>> -; CHECK-NEXT: [[B:%.*]] = shl <2 x i32> [[A:%.*]], <i32 1, i32 1><br>
>> -; CHECK-NEXT: ret <2 x i32> [[B]]<br>
>> +; CHECK-NEXT: [[B_NEG_NEG:%.*]] = shl <2 x i32> [[A:%.*]], <i32 1, i32 1><br>
>> +; CHECK-NEXT: ret <2 x i32> [[B_NEG_NEG]]<br>
>> ;<br>
>> %B = sub <2 x i32> <i32 1, i32 1>, %A<br>
>> %C = shl <2 x i32> %B, <i32 1, i32 1><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>