[llvm] 868abc4 - Revert "[GVN] Refactor handling of pointer-select in GVN pass"
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 16 06:57:02 PST 2023
Reminder to please always mention the reason for the revert/recommit
in the commit message.
On Mon, Jan 16, 2023 at 3:14 PM Sergey Kachkov via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>
> Author: Sergey Kachkov
> Date: 2023-01-16T15:13:17+03:00
> New Revision: 868abc471d1060175f71d2749badd864b1ba4f92
>
> URL: https://github.com/llvm/llvm-project/commit/868abc471d1060175f71d2749badd864b1ba4f92
> DIFF: https://github.com/llvm/llvm-project/commit/868abc471d1060175f71d2749badd864b1ba4f92.diff
>
> LOG: Revert "[GVN] Refactor handling of pointer-select in GVN pass"
>
> This reverts commit fc7cdaa373308ce3d72218b4d80101ae19850a6c.
>
> Added:
>
>
> Modified:
> llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h
> llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
> llvm/lib/Transforms/Scalar/GVN.cpp
> llvm/test/Transforms/GVN/PRE/pre-load-through-select.ll
>
> Removed:
>
>
>
> ################################################################################
> diff --git a/llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h b/llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h
> index b99807a25b775..24b4673a47e25 100644
> --- a/llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h
> +++ b/llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h
> @@ -79,10 +79,6 @@ class MemDepResult {
> /// the calls are the same.
> Def,
>
> - /// This marker indicates that the query has dependency from select
> - /// instruction.
> - Select,
> -
> /// This marker indicates that the query has no known dependency in the
> /// specified block.
> ///
> @@ -110,7 +106,6 @@ class MemDepResult {
> DepType, PointerSumTypeMember<Invalid, Instruction *>,
> PointerSumTypeMember<Clobber, Instruction *>,
> PointerSumTypeMember<Def, Instruction *>,
> - PointerSumTypeMember<Select, Instruction *>,
> PointerSumTypeMember<Other, PointerEmbeddedInt<OtherType, 3>>>;
> ValueTy Value;
>
> @@ -129,9 +124,6 @@ class MemDepResult {
> assert(Inst && "Clobber requires inst");
> return MemDepResult(ValueTy::create<Clobber>(Inst));
> }
> - static MemDepResult getSelect(Instruction *Inst) {
> - return MemDepResult(ValueTy::create<Select>(Inst));
> - }
> static MemDepResult getNonLocal() {
> return MemDepResult(ValueTy::create<Other>(NonLocal));
> }
> @@ -150,13 +142,6 @@ class MemDepResult {
> /// definition dependency.
> bool isDef() const { return Value.is<Def>(); }
>
> - /// Tests if this MemDepResult represents a query that is an instruction
> - /// select dependency.
> - bool isSelect() const { return Value.is<Select>(); }
> -
> - /// Tests if this MemDepResult represents a local query (Clobber/Def/Select).
> - bool isLocal() const { return isClobber() || isDef() || isSelect(); }
> -
> /// Tests if this MemDepResult represents a query that is transparent to the
> /// start of the block, but where a non-local hasn't been done.
> bool isNonLocal() const {
> @@ -185,8 +170,6 @@ class MemDepResult {
> return Value.cast<Clobber>();
> case Def:
> return Value.cast<Def>();
> - case Select:
> - return Value.cast<Select>();
> case Other:
> return nullptr;
> }
>
> diff --git a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
> index 960679ba56f79..7a13e7a641f32 100644
> --- a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
> +++ b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
> @@ -592,11 +592,6 @@ MemDepResult MemoryDependenceResults::getSimplePointerDependencyFrom(
> return MemDepResult::getDef(Inst);
> }
>
> - // If we found a select instruction for MemLoc pointer, return it as
> - // Select dependency.
> - if (isa<SelectInst>(Inst) && MemLoc.Ptr == Inst)
> - return MemDepResult::getSelect(Inst);
> -
> if (isInvariantLoad)
> continue;
>
> @@ -967,7 +962,7 @@ MemDepResult MemoryDependenceResults::getNonLocalInfoForBlock(
> // If the block has a dependency (i.e. it isn't completely transparent to
> // the value), remember the reverse association because we just added it
> // to Cache!
> - if (!Dep.isLocal())
> + if (!Dep.isDef() && !Dep.isClobber())
> return Dep;
>
> // Keep the ReverseNonLocalPtrDeps map up to date so we can efficiently
>
> diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
> index e542efb45e269..f9fdd7783820b 100644
> --- a/llvm/lib/Transforms/Scalar/GVN.cpp
> +++ b/llvm/lib/Transforms/Scalar/GVN.cpp
> @@ -1120,26 +1120,31 @@ static void reportMayClobberedLoad(LoadInst *Load, MemDepResult DepInfo,
> /// 1. The pointer select (\p Address) must be defined in \p DepBB.
> /// 2. Both value operands of the pointer select must be loaded in the same
> /// basic block, before the pointer select.
> -/// 3. There must be no instructions between the found loads and \p Sel that may
> +/// 3. There must be no instructions between the found loads and \p End that may
> /// clobber the loads.
> static std::optional<AvailableValue>
> -tryToConvertLoadOfPtrSelect(SelectInst *Sel, Type *LoadTy, DominatorTree &DT,
> +tryToConvertLoadOfPtrSelect(BasicBlock *DepBB, BasicBlock::iterator End,
> + Value *Address, Type *LoadTy, DominatorTree &DT,
> AAResults *AA) {
> +
> + auto *Sel = dyn_cast_or_null<SelectInst>(Address);
> + if (!Sel || DepBB != Sel->getParent())
> + return std::nullopt;
> +
> LoadInst *L1 = findDominatingLoad(Sel->getOperand(1), LoadTy, Sel, DT);
> LoadInst *L2 = findDominatingLoad(Sel->getOperand(2), LoadTy, Sel, DT);
> if (!L1 || !L2)
> return std::nullopt;
>
> // Ensure there are no accesses that may modify the locations referenced by
> - // either L1 or L2 between L1, L2 and the specified Sel instruction.
> + // either L1 or L2 between L1, L2 and the specified End iterator.
> Instruction *EarlierLoad = L1->comesBefore(L2) ? L1 : L2;
> MemoryLocation L1Loc = MemoryLocation::get(L1);
> MemoryLocation L2Loc = MemoryLocation::get(L2);
> - if (any_of(make_range(EarlierLoad->getIterator(), Sel->getIterator()),
> - [&](Instruction &I) {
> - return isModSet(AA->getModRefInfo(&I, L1Loc)) ||
> - isModSet(AA->getModRefInfo(&I, L2Loc));
> - }))
> + if (any_of(make_range(EarlierLoad->getIterator(), End), [&](Instruction &I) {
> + return isModSet(AA->getModRefInfo(&I, L1Loc)) ||
> + isModSet(AA->getModRefInfo(&I, L2Loc));
> + }))
> return std::nullopt;
>
> return AvailableValue::getSelect(Sel);
> @@ -1148,21 +1153,20 @@ tryToConvertLoadOfPtrSelect(SelectInst *Sel, Type *LoadTy, DominatorTree &DT,
> std::optional<AvailableValue>
> GVNPass::AnalyzeLoadAvailability(LoadInst *Load, MemDepResult DepInfo,
> Value *Address) {
> - assert(Load->isUnordered() && "rules below are incorrect for ordered access");
> - assert(DepInfo.isLocal() && "expected a local dependence");
> -
> - Instruction *DepInst = DepInfo.getInst();
> - if (DepInfo.isSelect()) {
> - // Check if load with Addr dependent from select can be converted to select
> - // between load values. There must be no instructions between the found
> - // loads and DepInst that may clobber the loads.
> - assert(isa<SelectInst>(DepInst) && "Dependency must be select instruction");
> - auto *Sel = cast<SelectInst>(DepInst);
> - return tryToConvertLoadOfPtrSelect(Sel, Load->getType(), getDominatorTree(),
> - getAliasAnalysis());
> + if (!DepInfo.isDef() && !DepInfo.isClobber()) {
> + assert(isa<SelectInst>(Address));
> + return tryToConvertLoadOfPtrSelect(Load->getParent(), Load->getIterator(),
> + Address, Load->getType(),
> + getDominatorTree(), getAliasAnalysis());
> }
>
> + assert((DepInfo.isDef() || DepInfo.isClobber()) &&
> + "expected a local dependence");
> + assert(Load->isUnordered() && "rules below are incorrect for ordered access");
> +
> const DataLayout &DL = Load->getModule()->getDataLayout();
> +
> + Instruction *DepInst = DepInfo.getInst();
> if (DepInfo.isClobber()) {
> // If the dependence is to a store that writes to a superset of the bits
> // read by the load, we can extract the bits we need for the load from the
> @@ -1294,15 +1298,24 @@ void GVNPass::AnalyzeLoadAvailability(LoadInst *Load, LoadDepVect &Deps,
> continue;
> }
>
> - if (!DepInfo.isLocal()) {
> + // The address being loaded in this non-local block may not be the same as
> + // the pointer operand of the load if PHI translation occurs. Make sure
> + // to consider the right address.
> + Value *Address = Dep.getAddress();
> +
> + if (!DepInfo.isDef() && !DepInfo.isClobber()) {
> + if (auto R = tryToConvertLoadOfPtrSelect(
> + DepBB, DepBB->end(), Address, Load->getType(), getDominatorTree(),
> + getAliasAnalysis())) {
> + ValuesPerBlock.push_back(
> + AvailableValueInBlock::get(DepBB, std::move(*R)));
> + continue;
> + }
> UnavailableBlocks.push_back(DepBB);
> continue;
> }
>
> - // The address being loaded in this non-local block may not be the same as
> - // the pointer operand of the load if PHI translation occurs. Make sure
> - // to consider the right address.
> - if (auto AV = AnalyzeLoadAvailability(Load, DepInfo, Dep.getAddress())) {
> + if (auto AV = AnalyzeLoadAvailability(Load, DepInfo, Address)) {
> // subtlety: because we know this was a non-local dependency, we know
> // it's safe to materialize anywhere between the instruction within
> // DepInfo and the end of it's block.
> @@ -2030,8 +2043,9 @@ bool GVNPass::processLoad(LoadInst *L) {
> if (Dep.isNonLocal())
> return processNonLocalLoad(L);
>
> + Value *Address = L->getPointerOperand();
> // Only handle the local case below
> - if (!Dep.isLocal()) {
> + if (!Dep.isDef() && !Dep.isClobber() && !isa<SelectInst>(Address)) {
> // This might be a NonFuncLocal or an Unknown
> LLVM_DEBUG(
> // fast print dep, using operator<< on instruction is too slow.
> @@ -2040,7 +2054,7 @@ bool GVNPass::processLoad(LoadInst *L) {
> return false;
> }
>
> - auto AV = AnalyzeLoadAvailability(L, Dep, L->getPointerOperand());
> + auto AV = AnalyzeLoadAvailability(L, Dep, Address);
> if (!AV)
> return false;
>
>
> diff --git a/llvm/test/Transforms/GVN/PRE/pre-load-through-select.ll b/llvm/test/Transforms/GVN/PRE/pre-load-through-select.ll
> index e53aa97269207..af1a13a1cbbb8 100644
> --- a/llvm/test/Transforms/GVN/PRE/pre-load-through-select.ll
> +++ b/llvm/test/Transforms/GVN/PRE/pre-load-through-select.ll
> @@ -236,15 +236,13 @@ define i32 @test_pointer_phi_select_simp_store_clobber_3(ptr %a, ptr %b, ptr %c,
> ; CHECK-NEXT: [[L_1:%.*]] = load i32, ptr [[A:%.*]], align 4
> ; CHECK-NEXT: [[L_2:%.*]] = load i32, ptr [[B:%.*]], align 4
> ; CHECK-NEXT: [[CMP_I_I_I:%.*]] = icmp ult i32 [[L_1]], [[L_2]]
> -; CHECK-NEXT: [[SELECT:%.*]] = select i1 [[CMP_I_I_I]], i32 [[L_1]], i32 [[L_2]]
> ; CHECK-NEXT: [[MIN_SELECT:%.*]] = select i1 [[CMP_I_I_I]], ptr [[A]], ptr [[B]]
> ; CHECK-NEXT: br label [[EXIT:%.*]]
> ; CHECK: else:
> -; CHECK-NEXT: [[RES_2_PRE:%.*]] = load i32, ptr [[A]], align 4
> ; CHECK-NEXT: br label [[EXIT]]
> ; CHECK: exit:
> -; CHECK-NEXT: [[RES_2:%.*]] = phi i32 [ [[SELECT]], [[THEN]] ], [ [[RES_2_PRE]], [[ELSE]] ]
> ; CHECK-NEXT: [[P:%.*]] = phi ptr [ [[MIN_SELECT]], [[THEN]] ], [ [[A]], [[ELSE]] ]
> +; CHECK-NEXT: [[RES_2:%.*]] = load i32, ptr [[P]], align 4
> ; CHECK-NEXT: ret i32 [[RES_2]]
> ;
> entry:
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list