[llvm] 70472f3 - [Reassociate] Convert `add`-like `or`'s into an `add`'s to allow reassociation

Tue Nov 3 12:52:44 PST 2020

On Tue, Nov 3, 2020 at 11:34 PM Nikita Popov <nikita.ppv at gmail.com> wrote:
>
> On Tue, Nov 3, 2020 at 8:31 PM Roman Lebedev via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>>
>> Author: Roman Lebedev
>> Date: 2020-11-03T22:30:51+03:00
>> New Revision: 70472f34b289d663a9f70740dfc9ae32fc89e077
>>
>> URL: https://github.com/llvm/llvm-project/commit/70472f34b289d663a9f70740dfc9ae32fc89e077
>> DIFF: https://github.com/llvm/llvm-project/commit/70472f34b289d663a9f70740dfc9ae32fc89e077.diff
>>
>> LOG: [Reassociate] Convert `add`-like `or`'s into an `add`'s to allow reassociation
>>
>> InstCombine is quite aggressive in doing the opposite transform,
>> folding `add` of operands with no common bits set into an `or`,
>> and that not many things support that new pattern..
>>
>> In this case, teaching Reassociate about it is easy,
>> there's preexisting art for `sub`/`shl`:
>> just convert such an `or` into an `add`:
>> https://rise4fun.com/Alive/Xlyv
>>
>> Added:
>>
>>
>> Modified:
>>     llvm/lib/Transforms/Scalar/Reassociate.cpp
>>     llvm/test/Transforms/Reassociate/add-like-or.ll
>>
>> Removed:
>>
>>
>>
>> ################################################################################
>> diff  --git a/llvm/lib/Transforms/Scalar/Reassociate.cpp b/llvm/lib/Transforms/Scalar/Reassociate.cpp
>> index ba7f367267fe..164971c916d4 100644
>> --- a/llvm/lib/Transforms/Scalar/Reassociate.cpp
>> +++ b/llvm/lib/Transforms/Scalar/Reassociate.cpp
>> @@ -920,6 +920,24 @@ static Value *NegateValue(Value *V, Instruction *BI,
>>    return NewNeg;
>>  }
>>
>> +/// If we have (X|Y), and iff X and Y have no common bits set,
>> +/// transform this into (X+Y) to allow arithmetics reassociation.
>> +static BinaryOperator *ConvertOrWithNoCommonBitsToAdd(Instruction *Or) {
>> +  // Convert an or into an add.
>> +  BinaryOperator *New =
>> +      CreateAdd(Or->getOperand(0), Or->getOperand(1), "", Or, Or);
>> +  New->setHasNoSignedWrap();
>> +  New->setHasNoUnsignedWrap();
>> +  New->takeName(Or);
>> +
>> +  // Everyone now refers to the add instruction.
>> +  Or->replaceAllUsesWith(New);
>> +  New->setDebugLoc(Or->getDebugLoc());
>> +
>> +  LLVM_DEBUG(dbgs() << "Converted or into an add: " << *New << '\n');
>> +  return New;
>> +}
>> +
>>  /// Return true if we should break up this subtract of X-Y into (X + -Y).
>>  static bool ShouldBreakUpSubtract(Instruction *Sub) {
>>    // If this is a negation, we can't split it up!
>> @@ -2116,6 +2134,18 @@ void ReassociatePass::OptimizeInst(Instruction *I) {
>>    if (I->getType()->isIntegerTy(1))
>>      return;
>>
>> +  // If this is a bitwise or instruction of operands
>> +  // with no common bits set, convert it to X+Y.
>> +  if (I->getOpcode() == Instruction::Or &&
>> +      haveNoCommonBitsSet(I->getOperand(0), I->getOperand(1),
>> +                          I->getModule()->getDataLayout(), /*AC=*/nullptr, I,
>> +                          /*DT=*/nullptr)) {
>> +    Instruction *NI = ConvertOrWithNoCommonBitsToAdd(I);
>> +    RedoInsts.insert(I);
>> +    MadeChange = true;
>> +    I = NI;
>> +  }
>> +
>>    // If this is a subtract instruction which is not already in negate form,
>>    // see if we can convert it to X+-Y.
>>    if (I->getOpcode() == Instruction::Sub) {
>>
>> diff  --git a/llvm/test/Transforms/Reassociate/add-like-or.ll b/llvm/test/Transforms/Reassociate/add-like-or.ll
>> index 25f2c0563da9..13477ad5a491 100644
>> --- a/llvm/test/Transforms/Reassociate/add-like-or.ll
>> +++ b/llvm/test/Transforms/Reassociate/add-like-or.ll
>> @@ -17,7 +17,7 @@ define i32 @test1(i32 %a, i32 %b) {
>>  define i32 @test2(i32 %x) {
>>  ; CHECK-LABEL: @test2(
>>  ; CHECK-NEXT:    [[X_NUMLZ:%.*]] = tail call i32 @llvm.ctlz.i32(i32 [[X:%.*]], i1 true), [[RNG0:!range !.*]]
>> -; CHECK-NEXT:    [[RES:%.*]] = or i32 [[X_NUMLZ]], -32
>> +; CHECK-NEXT:    [[RES:%.*]] = add nuw nsw i32 [[X_NUMLZ]], -32
>>  ; CHECK-NEXT:    ret i32 [[RES]]
>>  ;
>>    %x.numlz = tail call i32 @llvm.ctlz.i32(i32 %x, i1 true), !range !0
>> @@ -29,9 +29,8 @@ define i32 @test2(i32 %x) {
>>  define i32 @test3(i32 %x, i32 %bit) {
>>  ; CHECK-LABEL: @test3(
>>  ; CHECK-NEXT:    [[X_NUMLZ:%.*]] = tail call i32 @llvm.ctlz.i32(i32 [[X:%.*]], i1 true), [[RNG0]]
>> -; CHECK-NEXT:    [[ZERO_MINUS_X_NUMACTIVEBITS:%.*]] = or i32 [[X_NUMLZ]], -32
>> -; CHECK-NEXT:    [[BIT_PLUS_ONE:%.*]] = add i32 [[BIT:%.*]], 1
>> -; CHECK-NEXT:    [[RES:%.*]] = add i32 [[BIT_PLUS_ONE]], [[ZERO_MINUS_X_NUMACTIVEBITS]]
>> +; CHECK-NEXT:    [[BIT_PLUS_ONE:%.*]] = add i32 [[BIT:%.*]], -31
>> +; CHECK-NEXT:    [[RES:%.*]] = add i32 [[BIT_PLUS_ONE]], [[X_NUMLZ]]
>>  ; CHECK-NEXT:    ret i32 [[RES]]
>>  ;
>>    %x.numlz = tail call i32 @llvm.ctlz.i32(i32 %x, i1 true), !range !0
>
>
> Not familiar with this transform, so possibly stupid question:

> The way I'm reading this change, you are unconditionally reverting the InstCombine canonicalization.
Correct.

> The other transform you mention (shl -> mul) first checks whether doing this will result in reassociation opportunities.

> I'm wondering if the same shouldn't be done here as well?

As far as i can tell, either the reassociation will happen, or
instcombine will uglify that back:
https://github.com/llvm/llvm-project/blob/88241ffb5636ebc0579d3ab8eeec78446a769c54/llvm/test/Other/opt-O3-pipeline.ll#L107-L132
In fact, i recall adding some transforms to instcombine to do just that.

Unless doing it eagerly in Reassociate causes trouble for Reassociate itself
(https://github.com/llvm/llvm-project/commit/8db41e9dbd141d9904da6e29d098a49eed933400),
i'm not really sure there's much difference in doing the
'profitability' check before or after..

I believe, another example of that question is SROA's AggLoadStoreRewriter.

> Regards,
> Nikita
Roman