From 522d679e5924e34b467e5b16ca0d6c7df4f8f2be Mon Sep 17 00:00:00 2001 From: Roman Lebedev Date: Tue, 7 Jul 2020 00:25:19 +0300 Subject: [PATCH] [Scalarizer] Centralize instruction DCE As reported in https://reviews.llvm.org/D83101#2133062 the new visitInsertElementInst()/visitExtractElementInst() functionality is causing miscompiles (previously-crashing test added) It is due to the fact how the infra of Scalarizer is dealing with DCE, it was not updated or was it ready for such scalar value forwarding. It always assumed that the moment we "scalarized" something, it can go away, and did so with prejudice. But that is no longer safe/okay to do. Instead, let's prevent it from ever shooting itself into foot, and let's just accumulate the instructions-to-be-deleted in a vector, and collectively cleanup (those that are *actually* dead) them all at the end. All existing tests are not reporting any new garbage leftovers, but maybe it's test coverage issue. --- lib/Transforms/Scalar/Scalarizer.cpp | 17 +++++++++-------- test/Transforms/Scalarizer/basic.ll | 13 +++++++++++++ test/Transforms/Scalarizer/crash-bug.ll | 1 - .../Scalarizer/phi-unreachable-pred.ll | 7 ++----- 4 files changed, 24 insertions(+), 14 deletions(-) diff --git a/lib/Transforms/Scalar/Scalarizer.cpp b/lib/Transforms/Scalar/Scalarizer.cpp index 5cac4dca8cf..e8fea501b1d 100644 --- a/lib/Transforms/Scalar/Scalarizer.cpp +++ b/lib/Transforms/Scalar/Scalarizer.cpp @@ -22,8 +22,8 @@ #include "llvm/IR/BasicBlock.h" #include "llvm/IR/Constants.h" #include "llvm/IR/DataLayout.h" -#include "llvm/IR/Dominators.h" #include "llvm/IR/DerivedTypes.h" +#include "llvm/IR/Dominators.h" #include "llvm/IR/Function.h" #include "llvm/IR/IRBuilder.h" #include "llvm/IR/InstVisitor.h" @@ -41,6 +41,7 @@ #include "llvm/Support/CommandLine.h" #include "llvm/Support/MathExtras.h" #include "llvm/Transforms/Scalar.h" +#include "llvm/Transforms/Utils/Local.h" #include #include #include @@ -222,6 +223,8 @@ private: ScatterMap Scattered; GatherList Gathered; + SmallVector PotentiallyDeadInstrs; + unsigned ParallelLoopAccessMDKind; DominatorTree *DT; @@ -383,11 +386,6 @@ Scatterer ScalarizerVisitor::scatter(Instruction *Point, Value *V) { // so that we can avoid creating the gathered form if all uses of Op are // replaced with uses of CV. void ScalarizerVisitor::gather(Instruction *Op, const ValueVector &CV) { - // Since we're not deleting Op yet, stub out its operands, so that it - // doesn't make anything live unnecessarily. - for (unsigned I = 0, E = Op->getNumOperands(); I != E; ++I) - Op->setOperand(I, UndefValue::get(Op->getOperand(I)->getType())); - transferMetadataAndIRFlags(Op, CV); // If we already have a scattered form of Op (created from ExtractElements @@ -402,7 +400,7 @@ void ScalarizerVisitor::gather(Instruction *Op, const ValueVector &CV) { Instruction *Old = cast(V); CV[I]->takeName(Old); Old->replaceAllUsesWith(CV[I]); - Old->eraseFromParent(); + PotentiallyDeadInstrs.emplace_back(Old); } } SV = CV; @@ -950,10 +948,13 @@ bool ScalarizerVisitor::finish() { Res->takeName(Op); Op->replaceAllUsesWith(Res); } - Op->eraseFromParent(); + PotentiallyDeadInstrs.emplace_back(Op); } Gathered.clear(); Scattered.clear(); + + RecursivelyDeleteTriviallyDeadInstructionsPermissive(PotentiallyDeadInstrs); + return true; } diff --git a/test/Transforms/Scalarizer/basic.ll b/test/Transforms/Scalarizer/basic.ll index 2c7b6a6b588..239cdd660a3 100644 --- a/test/Transforms/Scalarizer/basic.ll +++ b/test/Transforms/Scalarizer/basic.ll @@ -539,6 +539,19 @@ define <2 x float> @f22(<2 x float> %x, <2 x float> %y, <2 x float> %z) { ret <2 x float> %res } +; See https://reviews.llvm.org/D83101#2133062 +define <2 x i32> @f23_crash(<2 x i32> %srcvec, i32 %v1) { +; CHECK-LABEL: @f23_crash( +; CHECK: %1 = extractelement <2 x i32> %srcvec, i32 0 +; CHECK: %t1.upto0 = insertelement <2 x i32> undef, i32 %1, i32 0 +; CHECK: %t1 = insertelement <2 x i32> %t1.upto0, i32 %v1, i32 1 +; CHECK: ret <2 x i32> %t1 + %v0 = extractelement <2 x i32> %srcvec, i32 0 + %t0 = insertelement <2 x i32> undef, i32 %v0, i32 0 + %t1 = insertelement <2 x i32> %t0, i32 %v1, i32 1 + ret <2 x i32> %t1 +} + !0 = !{ !"root" } !1 = !{ !"set1", !0 } !2 = !{ !"set2", !0 } diff --git a/test/Transforms/Scalarizer/crash-bug.ll b/test/Transforms/Scalarizer/crash-bug.ll index d0d01956497..d581707971e 100644 --- a/test/Transforms/Scalarizer/crash-bug.ll +++ b/test/Transforms/Scalarizer/crash-bug.ll @@ -15,7 +15,6 @@ bb2: ; preds = %bb1 bb1: ; preds = %bb2, %0 %bb1_vec = phi <2 x i16> [ , %0 ], [ %bb2_vec, %bb2 ] ;CHECK: bb1: -;CHECK: %bb1_vec.i0 = phi i16 [ 100, %0 ], [ 0, %bb2 ] ;CHECK: %bb2_vec.i1 = phi i16 [ 200, %0 ], [ %bb2_vec.i1, %bb2 ] br i1 undef, label %bb3, label %bb2 diff --git a/test/Transforms/Scalarizer/phi-unreachable-pred.ll b/test/Transforms/Scalarizer/phi-unreachable-pred.ll index 8e89efb5d31..9cfffe3b977 100644 --- a/test/Transforms/Scalarizer/phi-unreachable-pred.ll +++ b/test/Transforms/Scalarizer/phi-unreachable-pred.ll @@ -11,11 +11,8 @@ define i16 @f1() { ; CHECK: for.cond: ; CHECK-NEXT: br i1 undef, label [[FOR_BODY:%.*]], label [[FOR_END]] ; CHECK: for.end: -; CHECK-NEXT: [[PHI_I0:%.*]] = phi i16 [ 1, [[ENTRY:%.*]] ], [ undef, [[FOR_COND]] ] -; CHECK-NEXT: [[PHI_I1:%.*]] = phi i16 [ 1, [[ENTRY]] ], [ undef, [[FOR_COND]] ] -; CHECK-NEXT: [[PHI_I2:%.*]] = phi i16 [ 1, [[ENTRY]] ], [ undef, [[FOR_COND]] ] -; CHECK-NEXT: [[PHI_I3:%.*]] = phi i16 [ 1, [[ENTRY]] ], [ undef, [[FOR_COND]] ] -; CHECK-NEXT: ret i16 [[PHI_I0]] +; CHECK-NEXT: [[EXTRACT:%.*]] = phi i16 [ 1, [[ENTRY:%.*]] ], [ undef, [[FOR_COND]] ] +; CHECK-NEXT: ret i16 [[EXTRACT]] ; entry: br label %for.end