[PATCH] D86018: [Attributor] fix AAValueConstantRange and IntegerRangeState implementation

[Kuter Dinel via Phabricator via llvm-commits]

kuter added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:6964
+      auto &OpAA =
+          A.getAAFor<AAValueConstantRange>(*this, IRPosition::value(OpV));
+      intersectKnown(
----------------
okura wrote:
> jdoerfert wrote:
> > okura wrote:
> > > uenoku wrote:
> > > > I'm not sure this propagation is necessary in `initialize` but at least, we need `TrackDpencdence = false` here. 
> > > For example, we can get [0,2) as a known range for `%a` instead of [0,inf) in the following case. And this propagation is needed for a value simplification in the case of `@potential_test3` in `potential.ll`.
> > > ```
> > > %a = zext i1 %bool to i32
> > > ```
> > > 
> > > > but at least, we need TrackDpencdence = false here.
> > > I'll fix that.
> > Why wouldn't that information be propagated during the update? Could you explain where it is lost?
> > Why wouldn't that information be propagated during the update?
> I thought this propagation in `initialize` is sufficient but I noticed that a known range would be better during the update. I'll move this into `updateImpl`.
> > Could you explain where it is lost?
> The dependency type in `calculateCastInst` is now `REQUIRED`. So If the operand's AA becomes invalid first, the known range is never propagated. 
Adding deps in initialize is problematic. Because initialize will not be called if the dependent attribute gets changed, only the updateAA will be called. In this instance you are only using the known information, it is not as big of a deal here but we should have the 'TrackDpencdence = false` here anyways.

Aside from this
Doing this kind of propagation in initialize can be  problematic in CGSCC pass since we call initialize on AAs that belong to functions other than functions that are in the SCC. I will send a patch that makes sure we don't manifest that information real soon. We should look at how this would effect the changes in the tests.


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

https://reviews.llvm.org/D86018