[ValueTracking] Use isSafeToSpeculativelyExecuteWithVariableReplaced() in more places #109149
Conversation
This file contains hidden or 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
…) in more places This replaces some uses of isSafeToSpeculativelyExecute() with isSafeToSpeculativelyExecuteWithVariableReplaced(), in cases where we are guarding against operand changes rather plain speculation. I believe that this is NFC with the current implementation of the function (as it only does something different from loads), but this makes us more defensive against future generalizations.
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Nikita Popov (nikic) ChangesThis replaces some uses of isSafeToSpeculativelyExecute() with isSafeToSpeculativelyExecuteWithVariableReplaced(), in cases where we are guarding against operand changes rather plain speculation. I believe that this is NFC with the current implementation of the function (as it only does something different from loads), but this makes us more defensive against future generalizations. Full diff: https://github.com/llvm/llvm-project/pull/109149.diff 3 Files Affected:
diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp
index 69e0627a89cc29..30dc4ae30dbfa5 100644
--- a/llvm/lib/Analysis/LazyValueInfo.cpp
+++ b/llvm/lib/Analysis/LazyValueInfo.cpp
@@ -1572,7 +1572,8 @@ ValueLatticeElement LazyValueInfoImpl::getValueAtUse(const Use &U) {
// This also disallows looking through phi nodes: If the phi node is part
// of a cycle, we might end up reasoning about values from different cycle
// iterations (PR60629).
- if (!CurrI->hasOneUse() || !isSafeToSpeculativelyExecute(CurrI))
+ if (!CurrI->hasOneUse() ||
+ !isSafeToSpeculativelyExecuteWithVariableReplaced(CurrI))
break;
CurrU = &*CurrI->use_begin();
}
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp b/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
index e018f80dc3b2c8..d9b4faff4c004d 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
@@ -476,7 +476,8 @@ Instruction *InstCombinerImpl::visitExtractElementInst(ExtractElementInst &EI) {
// it may make the operand poison.
BinaryOperator *BO;
if (match(SrcVec, m_BinOp(BO)) && cheapToScalarize(SrcVec, Index) &&
- (HasKnownValidIndex || isSafeToSpeculativelyExecute(BO))) {
+ (HasKnownValidIndex ||
+ isSafeToSpeculativelyExecuteWithVariableReplaced(BO))) {
// extelt (binop X, Y), Index --> binop (extelt X, Index), (extelt Y, Index)
Value *X = BO->getOperand(0), *Y = BO->getOperand(1);
Value *E0 = Builder.CreateExtractElement(X, Index);
@@ -2777,7 +2778,7 @@ Instruction *InstCombinerImpl::simplifyBinOpSplats(ShuffleVectorInst &SVI) {
return nullptr;
auto *BinOp = cast<BinaryOperator>(Op0);
- if (!isSafeToSpeculativelyExecute(BinOp))
+ if (!isSafeToSpeculativelyExecuteWithVariableReplaced(BinOp))
return nullptr;
Value *NewBO = Builder.CreateBinOp(BinOp->getOpcode(), X, Y);
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index aa3f3fbdaeffa0..1e606c51f72cdb 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2105,7 +2105,7 @@ Instruction *InstCombinerImpl::foldVectorBinop(BinaryOperator &Inst) {
// It may not be safe to reorder shuffles and things like div, urem, etc.
// because we may trap when executing those ops on unknown vector elements.
// See PR20059.
- if (!isSafeToSpeculativelyExecute(&Inst))
+ if (!isSafeToSpeculativelyExecuteWithVariableReplaced(&Inst))
return nullptr;
auto createBinOpShuffle = [&](Value *X, Value *Y, ArrayRef<int> M) {
|
|
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesThis replaces some uses of isSafeToSpeculativelyExecute() with isSafeToSpeculativelyExecuteWithVariableReplaced(), in cases where we are guarding against operand changes rather plain speculation. I believe that this is NFC with the current implementation of the function (as it only does something different from loads), but this makes us more defensive against future generalizations. Full diff: https://github.com/llvm/llvm-project/pull/109149.diff 3 Files Affected:
diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp
index 69e0627a89cc29..30dc4ae30dbfa5 100644
--- a/llvm/lib/Analysis/LazyValueInfo.cpp
+++ b/llvm/lib/Analysis/LazyValueInfo.cpp
@@ -1572,7 +1572,8 @@ ValueLatticeElement LazyValueInfoImpl::getValueAtUse(const Use &U) {
// This also disallows looking through phi nodes: If the phi node is part
// of a cycle, we might end up reasoning about values from different cycle
// iterations (PR60629).
- if (!CurrI->hasOneUse() || !isSafeToSpeculativelyExecute(CurrI))
+ if (!CurrI->hasOneUse() ||
+ !isSafeToSpeculativelyExecuteWithVariableReplaced(CurrI))
break;
CurrU = &*CurrI->use_begin();
}
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp b/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
index e018f80dc3b2c8..d9b4faff4c004d 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
@@ -476,7 +476,8 @@ Instruction *InstCombinerImpl::visitExtractElementInst(ExtractElementInst &EI) {
// it may make the operand poison.
BinaryOperator *BO;
if (match(SrcVec, m_BinOp(BO)) && cheapToScalarize(SrcVec, Index) &&
- (HasKnownValidIndex || isSafeToSpeculativelyExecute(BO))) {
+ (HasKnownValidIndex ||
+ isSafeToSpeculativelyExecuteWithVariableReplaced(BO))) {
// extelt (binop X, Y), Index --> binop (extelt X, Index), (extelt Y, Index)
Value *X = BO->getOperand(0), *Y = BO->getOperand(1);
Value *E0 = Builder.CreateExtractElement(X, Index);
@@ -2777,7 +2778,7 @@ Instruction *InstCombinerImpl::simplifyBinOpSplats(ShuffleVectorInst &SVI) {
return nullptr;
auto *BinOp = cast<BinaryOperator>(Op0);
- if (!isSafeToSpeculativelyExecute(BinOp))
+ if (!isSafeToSpeculativelyExecuteWithVariableReplaced(BinOp))
return nullptr;
Value *NewBO = Builder.CreateBinOp(BinOp->getOpcode(), X, Y);
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index aa3f3fbdaeffa0..1e606c51f72cdb 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2105,7 +2105,7 @@ Instruction *InstCombinerImpl::foldVectorBinop(BinaryOperator &Inst) {
// It may not be safe to reorder shuffles and things like div, urem, etc.
// because we may trap when executing those ops on unknown vector elements.
// See PR20059.
- if (!isSafeToSpeculativelyExecute(&Inst))
+ if (!isSafeToSpeculativelyExecuteWithVariableReplaced(&Inst))
return nullptr;
auto createBinOpShuffle = [&](Value *X, Value *Y, ArrayRef<int> M) {
|
goldsteinn
reviewed
Sep 18, 2024
| @@ -476,7 +476,8 @@ Instruction *InstCombinerImpl::visitExtractElementInst(ExtractElementInst &EI) { | |||
| // it may make the operand poison. | |||
| BinaryOperator *BO; | |||
| if (match(SrcVec, m_BinOp(BO)) && cheapToScalarize(SrcVec, Index) && | |||
| (HasKnownValidIndex || isSafeToSpeculativelyExecute(BO))) { | |||
| (HasKnownValidIndex || | |||
| isSafeToSpeculativelyExecuteWithVariableReplaced(BO))) { | |||
There was a problem hiding this comment.
Why is it necessary here? Isn't this transform change BO to handle strictly a subset of what it was previously?
There was a problem hiding this comment.
See the comment above this code: We may replace the operand with poison if the extractelement index is out of bounds.
goldsteinn
reviewed
Sep 18, 2024
This was referenced Sep 19, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/8131 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/6036 Here is the relevant piece of the build log for the reference |
tmsri
pushed a commit
to tmsri/llvm-project
that referenced
this pull request
Sep 19, 2024
…) in more places (llvm#109149) This replaces some uses of isSafeToSpeculativelyExecute() with isSafeToSpeculativelyExecuteWithVariableReplaced(), in cases where we are guarding against operand changes rather plain speculation. I believe that this is NFC with the current implementation of the function (as it only does something different from loads), but this makes us more defensive against future generalizations.
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.
This replaces some uses of isSafeToSpeculativelyExecute() with isSafeToSpeculativelyExecuteWithVariableReplaced(), in cases where we are guarding against operand changes rather plain speculation.
I believe that this is NFC with the current implementation of the function (as it only does something different from loads), but this makes us more defensive against future generalizations.