[PATCH] D71620: [Attributor] AAValueConstantRange: Value range analysis using constant range

[Hideto Ueno via Phabricator via llvm-commits]

uenoku marked an inline comment as done.
uenoku added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1421
+    Known = Known.intersectWith(R);
+  }
+
----------------
jdoerfert wrote:
> I think you need to adjust Assumed too otherwise it might not be a proper subset of Known anymore.
Fixed.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1440
+    unionAssumed(R);
+  }
+
----------------
jdoerfert wrote:
> Do we need to intersect the known range here as well?
In my understanding, "Clamp" should only modify the assumption.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4975
+    if (AssumedConstantRange.isFullSet())
+      return ChangeStatus::UNCHANGED;
+
----------------
jdoerfert wrote:
> Shouldn't this imply "an invalid state" (= the worst possible state)? If so, it should never be make it to this point (which you should assert instead).
Yes, I changed to assert.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5129
+                  }
+              }
+            T.indicatePessimisticFixpoint();
----------------
jdoerfert wrote:
> I don't think we need to look at all loops here.
> The problem is also that `getSCEVAtScope` might silently fail to actually compute the exit value and instead continue computation with the original one.
> 
> I think this is OK already: `SE->getUnsignedRange(SE->getSCEV())`
> 
> 
Yes, you are right. I changed to use `SE->getUnsignedRange(SE->getSCEV())` as known range. And if a program point  `(Instrution *I)`is given, `SCEV->getSCEVAtScope(I->getParent())` is used.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71620/new/

https://reviews.llvm.org/D71620