[SCCPSolver] Move getValueState to later

[aokblast]

If the IV is overDefined, there is no reason for us to init V0State.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (aokblast)

Changes

If the IV is overDefined, there is no reason for us to init V0State.

---

Full diff: https://github.com/llvm/llvm-project/pull/175307.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Utils/SCCPSolver.cpp (+2-3)

diff --git a/llvm/lib/Transforms/Utils/SCCPSolver.cpp b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
index 021bf0618754a..b5b7a7fbb587b 100644
--- a/llvm/lib/Transforms/Utils/SCCPSolver.cpp
+++ b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
@@ -1661,14 +1661,13 @@ void SCCPInstVisitor::visitSelectInst(SelectInst &I) {
 
 // Handle Unary Operators.
 void SCCPInstVisitor::visitUnaryOperator(Instruction &I) {
-  ValueLatticeElement V0State = getValueState(I.getOperand(0));
-
   ValueLatticeElement &IV = ValueState[&I];
   // resolvedUndefsIn might mark I as overdefined. Bail out, even if we would
   // discover a concrete value later.
   if (IV.isOverdefined())
     return (void)markOverdefined(&I);
 
+  ValueLatticeElement V0State = getValueState(I.getOperand(0));
   // If something is unknown/undef, wait for it to resolve.
   if (V0State.isUnknownOrUndef())
     return;
@@ -1687,13 +1686,13 @@ void SCCPInstVisitor::visitFreezeInst(FreezeInst &I) {
   if (I.getType()->isStructTy())
     return (void)markOverdefined(&I);
 
-  ValueLatticeElement V0State = getValueState(I.getOperand(0));
   ValueLatticeElement &IV = ValueState[&I];
   // resolvedUndefsIn might mark I as overdefined. Bail out, even if we would
   // discover a concrete value later.
   if (IV.isOverdefined())
     return (void)markOverdefined(&I);
 
+  ValueLatticeElement V0State = getValueState(I.getOperand(0));
   // If something is unknown/undef, wait for it to resolve.
   if (V0State.isUnknownOrUndef())
     return;

[llvmbot]

@llvm/pr-subscribers-function-specialization

Author: None (aokblast)

Changes

If the IV is overDefined, there is no reason for us to init V0State.

---

Full diff: https://github.com/llvm/llvm-project/pull/175307.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Utils/SCCPSolver.cpp (+2-3)

diff --git a/llvm/lib/Transforms/Utils/SCCPSolver.cpp b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
index 021bf0618754a..b5b7a7fbb587b 100644
--- a/llvm/lib/Transforms/Utils/SCCPSolver.cpp
+++ b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
@@ -1661,14 +1661,13 @@ void SCCPInstVisitor::visitSelectInst(SelectInst &I) {
 
 // Handle Unary Operators.
 void SCCPInstVisitor::visitUnaryOperator(Instruction &I) {
-  ValueLatticeElement V0State = getValueState(I.getOperand(0));
-
   ValueLatticeElement &IV = ValueState[&I];
   // resolvedUndefsIn might mark I as overdefined. Bail out, even if we would
   // discover a concrete value later.
   if (IV.isOverdefined())
     return (void)markOverdefined(&I);
 
+  ValueLatticeElement V0State = getValueState(I.getOperand(0));
   // If something is unknown/undef, wait for it to resolve.
   if (V0State.isUnknownOrUndef())
     return;
@@ -1687,13 +1686,13 @@ void SCCPInstVisitor::visitFreezeInst(FreezeInst &I) {
   if (I.getType()->isStructTy())
     return (void)markOverdefined(&I);
 
-  ValueLatticeElement V0State = getValueState(I.getOperand(0));
   ValueLatticeElement &IV = ValueState[&I];
   // resolvedUndefsIn might mark I as overdefined. Bail out, even if we would
   // discover a concrete value later.
   if (IV.isOverdefined())
     return (void)markOverdefined(&I);
 
+  ValueLatticeElement V0State = getValueState(I.getOperand(0));
   // If something is unknown/undef, wait for it to resolve.
   if (V0State.isUnknownOrUndef())
     return;

[nikic]

This is not correct due to iterator invalidation. The subsequent markConstant() calls on IV may use an invalidated pointer.

[aokblast]

This is not correct due to iterator invalidation. The subsequent markConstant() calls on IV may use an invalidated pointer.

Hi! Thanks for your review. But I cannot get the point on how getValueState(I.getOperand(0)) affects the pointer of ValueState[&I]. I didn't see any code path that removes element and replaces with a new address of ValueState[&I] in visitUnaryOperator. The only possible way I think is &I.getOperand(0) == I. But is it possible? Could you please give me some hint? Thanks!

[dtcxzyw]

llvm-project/llvm/docs/ProgrammersManual.rst

Lines 2416 to 2417 in bf01761

	The iterators in a ``DenseMap`` are invalidated whenever an insertion occurs,
	unlike ``map``. Also, because ``DenseMap`` allocates space for a large number of

getValueState may insert new items into ValueState.

[aokblast]

llvm-project/llvm/docs/ProgrammersManual.rst

Lines 2416 to 2417 in bf01761

	The iterators in a ``DenseMap`` are invalidated whenever an insertion occurs,
	unlike ``map``. Also, because ``DenseMap`` allocates space for a large number of

getValueState may insert new items into ValueState.

Hi. Thanks for your explanation! I think we should move up V0State.isUnknownOrUndef() instead.

[aokblast]

Close this since I don't see any perceptabile acceleration after move upper.