[VectorCombine] Run load/extract scalarization after scalarizing store.

[fhahn]

Summary

Remove the insertelement instruction (via D98240) can enable further
load/extract scalarization opportunities. Try it again.

[fhahn]

Rebase and update to actually pass the first user.

[fhahn]

rebase and ping :) All pending patches should be committed now.

[spatel]

Some questions were raised about D98240, so we should deal with those before adding this.

[fhahn]

Rebase and ping :)

In D100302#2782012, @spatel wrote:

Some questions were raised about D98240, so we should deal with those before adding this.

I think those questions should be addressed by now and the issues raised by Alive2 should also be ironed out now.

[lebedev.ri]

I do not like the general direction VectorCombine seems to be going in.
Doing only a single pass and hoping that the second pass wouldn't do anything clearly leads to problems if (!Load->user_empty()) { is solving.
I would think we'd really want an instcombine-lite approach here, with a worklist..

[fhahn]

In D100302#2941173, @lebedev.ri wrote:

I do not like the general direction VectorCombine seems to be going in.
Doing only a single pass and hoping that the second pass wouldn't do anything clearly leads to problems if (!Load->user_empty()) { is solving.
I would think we'd really want an instcombine-lite approach here, with a worklist..

That's a fair point. But I think the same probably holds true fo somer other combines in VectorCombine. So should the whole pass just use a worklist?

[lebedev.ri]

In D100302#2949997, @fhahn wrote:

In D100302#2941173, @lebedev.ri wrote:

I do not like the general direction VectorCombine seems to be going in.
Doing only a single pass and hoping that the second pass wouldn't do anything clearly leads to problems if (!Load->user_empty()) { is solving.
I would think we'd really want an instcombine-lite approach here, with a worklist..

That's a fair point. But I think the same probably holds true fo somer other combines in VectorCombine. So should the whole pass just use a worklist?

That was my comment precisely.

[fhahn]

In D100302#2950029, @lebedev.ri wrote:

In D100302#2949997, @fhahn wrote:

In D100302#2941173, @lebedev.ri wrote:

I do not like the general direction VectorCombine seems to be going in.
Doing only a single pass and hoping that the second pass wouldn't do anything clearly leads to problems if (!Load->user_empty()) { is solving.
I would think we'd really want an instcombine-lite approach here, with a worklist..

That's a fair point. But I think the same probably holds true fo somer other combines in VectorCombine. So should the whole pass just use a worklist?

That was my comment precisely.

@spatel did you by any chance already think/consider this for VectorCombine? Or do you have any concerns?

[spatel]

In D100302#2950411, @fhahn wrote:

@spatel did you by any chance already think/consider this for VectorCombine? Or do you have any concerns?

No concerns. This has come up before (we were walking the instruction bottom-up at some point instead of top-down), but we just didn't have enough motivation with the existing transforms to justify implementing a worklist. If you want to try it, that would be great!

[fhahn]

In D100302#2950426, @spatel wrote:

In D100302#2950411, @fhahn wrote:

@spatel did you by any chance already think/consider this for VectorCombine? Or do you have any concerns?

No concerns. This has come up before (we were walking the instruction bottom-up at some point instead of top-down), but we just didn't have enough motivation with the existing transforms to justify implementing a worklist. If you want to try it, that would be great!

I put up a patch: D110171

I'll abandon the patch here for now.

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

if (!Load->isSimple() || Load->getParent() != SI->getParent() ||
       return false;
 
     Value *GEP = GetElementPtrInst::CreateInBounds(
         SI->getPointerOperand(), {ConstantInt::get(Idx->getType(), 0), Idx});
     Builder.Insert(GEP);
     StoreInst *NSI = Builder.CreateStore(NewElement, GEP);
     NSI->copyMetadata(*SI);
     replaceValue(I, *NSI);
+    auto *IEI = cast<Instruction>(SI->getOperand(0));
     // Need erasing the store manually.
     I.eraseFromParent();
+    // Removing the insertelement instruction may unlock further load/extract
+    // scalarization opportunities.
+    if (IEI->use_empty())
+      IEI->eraseFromParent();
+    scalarizeLoadExtract(*Load);
     return true;
   }
 
   return false;
 }
 
 /// Try to scalarize vector loads feeding extractelement instructions.
 bool VectorCombine::scalarizeLoadExtract(Instruction &I) {

llvm/test/Transforms/VectorCombine/AArch64/load-extract-insert-store-scalarization.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -vector-combine -S %s | FileCheck %s
 
 target triple = "arm64-apple-darwin"
 
 define void @test(i64 %i, <225 x double>* %A) {
 ; CHECK-LABEL: @test(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[LV:%.*]] = load <225 x double>, <225 x double>* [[A:%.*]], align 8
 ; CHECK-NEXT:    [[IDX_0:%.*]] = add nuw nsw i64 2, [[I:%.*]]
-; CHECK-NEXT:    [[EXT_0:%.*]] = extractelement <225 x double> [[LV]], i64 [[IDX_0]]
+; CHECK-NEXT:    [[TMP0:%.*]] = getelementptr inbounds <225 x double>, <225 x double>* [[A:%.*]], i32 0, i64 [[IDX_0]]
+; CHECK-NEXT:    [[EXT_0:%.*]] = load double, double* [[TMP0]], align 1
 ; CHECK-NEXT:    [[MUL:%.*]] = fmul double 2.000000e+01, [[EXT_0]]
-; CHECK-NEXT:    [[EXT_1:%.*]] = extractelement <225 x double> [[LV]], i64 [[I]]
+; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds <225 x double>, <225 x double>* [[A]], i32 0, i64 [[I]]
+; CHECK-NEXT:    [[EXT_1:%.*]] = load double, double* [[TMP1]], align 1
 ; CHECK-NEXT:    [[SUB:%.*]] = fsub double [[EXT_1]], [[MUL]]
-; CHECK-NEXT:    [[TMP0:%.*]] = getelementptr inbounds <225 x double>, <225 x double>* [[A]], i64 0, i64 [[I]]
-; CHECK-NEXT:    store double [[SUB]], double* [[TMP0]], align 8
+; CHECK-NEXT:    [[TMP2:%.*]] = getelementptr inbounds <225 x double>, <225 x double>* [[A]], i64 0, i64 [[I]]
+; CHECK-NEXT:    store double [[SUB]], double* [[TMP2]], align 8
 ; CHECK-NEXT:    ret void
 ;
 entry:
   %lv = load <225 x double>, <225 x double>* %A, align 8
   %idx.0 = add nuw nsw i64 2, %i
   %ext.0 = extractelement <225 x double> %lv, i64 %idx.0
   %mul = fmul double 20.0, %ext.0
   %ext.1 = extractelement <225 x double> %lv, i64 %i
   %sub = fsub double %ext.1, %mul
   %ins = insertelement <225 x double> %lv, double %sub, i64 %i
   store <225 x double> %ins, <225 x double>* %A, align 8
   ret void
 }