[NewGVN] Fix caching for OpIsSafeForPhiOfOps #98340
Merged
+116
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-transforms Author: None (ManuelJBrito) ChangesThe caching mechanism for 'OpIsSafeForPhiOfOps' is unsound. An operand is deemed unsafe for PhiOfOps if it depends on a phi that resides in the same block as the Phi block, i.e., where we are performing the PhiOfOps. This is to avoid having to materialize the translated subexpressions. To avoid redundant code walking, a cache is used to store these results. Note, however, that since the safety is specific to the Phi block, we cannot, in general, use the cached results for other blocks. This patch addresses this by having a cache per block instead of a single one for the entire function. closes #63335 Full diff: https://github.com/llvm/llvm-project/pull/98340.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 4cba196ed688a..864f1de7eea9d 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -529,7 +529,11 @@ class NewGVN {
// IR.
SmallPtrSet<const Instruction *, 8> PHINodeUses;
- DenseMap<const Value *, bool> OpSafeForPHIOfOps;
+ // The cached results, in general, are only valid for the specific block where
+ // they were computed. Therefore, we use a vector of caches indexed by a block
+ // identifier.
+ SmallVector<DenseMap<const Value *, bool>, 32> OpSafeForPHIOfOps;
+ unsigned CacheIdx;
// Map a temporary instruction we created to a parent block.
DenseMap<const Value *, BasicBlock *> TempToBlock;
@@ -2599,20 +2603,19 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
auto *I = Worklist.pop_back_val();
if (!isa<Instruction>(I))
continue;
-
- auto OISIt = OpSafeForPHIOfOps.find(I);
- if (OISIt != OpSafeForPHIOfOps.end())
+ auto OISIt = OpSafeForPHIOfOps[CacheIdx].find(I);
+ if (OISIt != OpSafeForPHIOfOps[CacheIdx].end())
return OISIt->second;
// Keep walking until we either dominate the phi block, or hit a phi, or run
// out of things to check.
if (DT->properlyDominates(getBlockForValue(I), PHIBlock)) {
- OpSafeForPHIOfOps.insert({I, true});
+ OpSafeForPHIOfOps[CacheIdx].insert({I, true});
continue;
}
// PHI in the same block.
if (isa<PHINode>(I) && getBlockForValue(I) == PHIBlock) {
- OpSafeForPHIOfOps.insert({I, false});
+ OpSafeForPHIOfOps[CacheIdx].insert({I, false});
return false;
}
@@ -2631,10 +2634,10 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
if (!isa<Instruction>(Op))
continue;
// Stop now if we find an unsafe operand.
- auto OISIt = OpSafeForPHIOfOps.find(OrigI);
- if (OISIt != OpSafeForPHIOfOps.end()) {
+ auto OISIt = OpSafeForPHIOfOps[CacheIdx].find(OrigI);
+ if (OISIt != OpSafeForPHIOfOps[CacheIdx].end()) {
if (!OISIt->second) {
- OpSafeForPHIOfOps.insert({I, false});
+ OpSafeForPHIOfOps[CacheIdx].insert({I, false});
return false;
}
continue;
@@ -2644,7 +2647,7 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
Worklist.push_back(cast<Instruction>(Op));
}
}
- OpSafeForPHIOfOps.insert({V, true});
+ OpSafeForPHIOfOps[CacheIdx].insert({V, true});
return true;
}
@@ -3296,7 +3299,9 @@ void NewGVN::verifyIterationSettled(Function &F) {
TouchedInstructions.set();
TouchedInstructions.reset(0);
- OpSafeForPHIOfOps.clear();
+ for (unsigned i = 0; i < OpSafeForPHIOfOps.size(); i++)
+ OpSafeForPHIOfOps[i].clear();
+ CacheIdx = 0;
iterateTouchedInstructions();
DenseSet<std::pair<const CongruenceClass *, const CongruenceClass *>>
EqualClasses;
@@ -3400,6 +3405,8 @@ void NewGVN::iterateTouchedInstructions() {
<< " because it is unreachable\n");
continue;
}
+ // Use the appropriate cache for "OpIsSafeForPHIOfOps".
+ CacheIdx = RPOOrdering.lookup(DT->getNode(CurrBlock)) - 1;
updateProcessedCount(CurrBlock);
}
// Reset after processing (because we may mark ourselves as touched when
@@ -3461,6 +3468,7 @@ bool NewGVN::runGVN() {
// Now a standard depth first ordering of the domtree is equivalent to RPO.
for (auto *DTN : depth_first(DT->getRootNode())) {
BasicBlock *B = DTN->getBlock();
+ OpSafeForPHIOfOps.emplace_back();
const auto &BlockRange = assignDFSNumbers(B, ICount);
BlockInstRange.insert({B, BlockRange});
ICount += BlockRange.second - BlockRange.first;
@@ -3479,6 +3487,8 @@ bool NewGVN::runGVN() {
LLVM_DEBUG(dbgs() << "Block " << getBlockName(&F.getEntryBlock())
<< " marked reachable\n");
ReachableBlocks.insert(&F.getEntryBlock());
+ // Use index corresponding to entry block.
+ CacheIdx = 0;
iterateTouchedInstructions();
verifyMemoryCongruency();
diff --git a/llvm/test/Transforms/NewGVN/cache-safe-phiofops.ll b/llvm/test/Transforms/NewGVN/cache-safe-phiofops.ll
new file mode 100644
index 0000000000000..d26d3f1190c24
--- /dev/null
+++ b/llvm/test/Transforms/NewGVN/cache-safe-phiofops.ll
@@ -0,0 +1,100 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=newgvn < %s | FileCheck %s
+
+define i32 @unsafe(i1 %arg, i32 %arg1) {
+; CHECK-LABEL: define i32 @unsafe(
+; CHECK-SAME: i1 [[ARG:%.*]], i32 [[ARG1:%.*]]) {
+; CHECK-NEXT: [[BB:.*:]]
+; CHECK-NEXT: br label %[[BB4:.*]]
+; CHECK: [[BB4]]:
+; CHECK-NEXT: br i1 [[ARG]], label %[[BB6:.*]], label %[[BB5:.*]]
+; CHECK: [[BB5]]:
+; CHECK-NEXT: br label %[[BB6]]
+; CHECK: [[BB6]]:
+; CHECK-NEXT: [[PHI:%.*]] = phi i32 [ [[ARG1]], %[[BB5]] ], [ 0, %[[BB4]] ]
+; CHECK-NEXT: [[SREM:%.*]] = srem i32 [[ARG1]], [[PHI]]
+; CHECK-NEXT: store i32 [[SREM]], ptr null, align 4
+; CHECK-NEXT: br i1 [[ARG]], label %[[BB8:.*]], label %[[BB7:.*]]
+; CHECK: [[BB7]]:
+; CHECK-NEXT: br i1 true, label %[[BB8]], label %[[BB5]]
+; CHECK: [[BB8]]:
+; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i32 [ [[ARG1]], %[[BB6]] ], [ 0, %[[BB7]] ]
+; CHECK-NEXT: [[PHI9:%.*]] = phi i32 [ 0, %[[BB7]] ], [ 1, %[[BB6]] ]
+; CHECK-NEXT: store i32 [[PHIOFOPS]], ptr null, align 4
+; CHECK-NEXT: br label %[[BB4]]
+;
+bb:
+ br label %bb4
+
+bb4: ; preds = %bb8, %bb
+ br i1 %arg, label %bb6, label %bb5
+
+bb5: ; preds = %bb7, %bb4
+ br label %bb6
+
+bb6: ; preds = %bb5, %bb4
+ %phi = phi i32 [ %arg1, %bb5 ], [ 0, %bb4 ]
+ %or = or i32 %phi, %arg1
+ %srem = srem i32 %or, %phi
+ store i32 %srem, ptr null, align 4
+ br i1 %arg, label %bb8, label %bb7
+
+bb7: ; preds = %bb6
+ br i1 true, label %bb8, label %bb5
+
+bb8: ; preds = %bb7, %bb6
+ %phi9 = phi i32 [ 0, %bb7 ], [ 1, %bb6 ]
+ %mul = mul i32 %phi9, %or
+ store i32 %mul, ptr null, align 4
+ br label %bb4
+}
+
+define i32 @unsafe_load(i1 %arg) {
+; CHECK-LABEL: define i32 @unsafe_load(
+; CHECK-SAME: i1 [[ARG:%.*]]) {
+; CHECK-NEXT: [[BB:.*:]]
+; CHECK-NEXT: br label %[[BB1:.*]]
+; CHECK: [[BB1]]:
+; CHECK-NEXT: br i1 [[ARG]], label %[[BB3:.*]], label %[[BB2:.*]]
+; CHECK: [[BB2]]:
+; CHECK-NEXT: br label %[[BB3]]
+; CHECK: [[BB3]]:
+; CHECK-NEXT: [[PHI4:%.*]] = phi i32 [ 1, %[[BB2]] ], [ 0, %[[BB1]] ]
+; CHECK-NEXT: [[LOAD:%.*]] = load i32, ptr null, align 4
+; CHECK-NEXT: [[SREM:%.*]] = srem i32 [[LOAD]], [[PHI4]]
+; CHECK-NEXT: store i32 [[SREM]], ptr null, align 4
+; CHECK-NEXT: br i1 [[ARG]], label %[[BB6:.*]], label %[[BB5:.*]]
+; CHECK: [[BB5]]:
+; CHECK-NEXT: br i1 true, label %[[BB6]], label %[[BB2]]
+; CHECK: [[BB6]]:
+; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i32 [ [[LOAD]], %[[BB3]] ], [ 0, %[[BB5]] ]
+; CHECK-NEXT: [[PHI7:%.*]] = phi i32 [ 0, %[[BB5]] ], [ 1, %[[BB3]] ]
+; CHECK-NEXT: store i32 [[PHIOFOPS]], ptr null, align 4
+; CHECK-NEXT: br label %[[BB1]]
+;
+bb:
+ br label %bb1
+
+bb1: ; preds = %bb6, %bb
+ br i1 %arg, label %bb3, label %bb2
+
+bb2: ; preds = %bb5, %bb1
+ %phi = phi i32 [ 1, %bb1 ], [ 0, %bb5 ]
+ br label %bb3
+
+bb3: ; preds = %bb2, %bb1
+ %phi4 = phi i32 [ %phi, %bb2 ], [ 0, %bb1 ]
+ %load = load i32, ptr null, align 4
+ %srem = srem i32 %load, %phi4
+ store i32 %srem, ptr null, align 4
+ br i1 %arg, label %bb6, label %bb5
+
+bb5: ; preds = %bb3
+ br i1 true, label %bb6, label %bb2
+
+bb6: ; preds = %bb5, %bb3
+ %phi7 = phi i32 [ 0, %bb5 ], [ 1, %bb3 ]
+ %mul = mul i32 %phi7, %load
+ store i32 %mul, ptr null, align 4
+ br label %bb1
+}
|
ManuelJBrito
force-pushed
the
CacheSafeForPhiOfOps
branch
from
3273391
to
f108e30
Compare
nunoplopes
approved these changes
Jul 11, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/50/builds/1018 Here is the relevant piece of the build log for the reference: |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
The caching mechanism for 'OpIsSafeForPhiOfOps' is unsound. An operand is deemed unsafe for PhiOfOps if it depends on a phi that resides in the same block as the Phi block, i.e., where we are performing the PhiOfOps. This is to avoid having to materialize the translated subexpressions. To avoid redundant code walking, a cache is used to store these results. Note, however, that since the safety is specific to the Phi block, we cannot, in general, use the cached results for other blocks. This patch addresses this by having a cache per block instead of a single one for the entire function. closes llvm#63335
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The caching mechanism for 'OpIsSafeForPhiOfOps' is unsound. An operand is deemed unsafe for PhiOfOps if it depends on a phi that resides in the same block as the Phi block, i.e., where we are performing the PhiOfOps. This is to avoid having to materialize the translated subexpressions. To avoid redundant code walking, a cache is used to store these results. Note, however, that since the safety is specific to the Phi block, we cannot, in general, use the cached results for other blocks.
This patch addresses this by having a cache per block instead of a single one for the entire function. closes #63335