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

[Shinji Okumura via Phabricator via llvm-commits]

okura added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1956
     Known = Known.unionWith(R);
-    Assumed = Assumed.unionWith(Known);
   }
----------------
jdoerfert wrote:
> This looks dangerous as we might end up with a known range that is not subsumed in the assumed range.
I'm not sure why a known range should be subsumed in the assumed range. Isn't it the other way around? For example, a known range and assumed range are initialized with full and empty respectively, and the assumed range is subsumed in the known range.
The following is another example. Let us assume the two returned values have the same state <full / [0,1)>. I think a clamped state for the returned position should be <full / [0,1)>. However, the clamped state is <full / full> with the current implementation.
```
i32 @foo(){
  ...
  ret i32 %bar
  ...
  ret i32 %baz
}


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:6964
+      auto &OpAA =
+          A.getAAFor<AAValueConstantRange>(*this, IRPosition::value(OpV));
+      intersectKnown(
----------------
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. 


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

https://reviews.llvm.org/D86018