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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1956
     Known = Known.unionWith(R);
-    Assumed = Assumed.unionWith(Known);
   }
----------------
uenoku wrote:
> okura wrote:
> > 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
> > }
> I think @okura is correct. I don't remember but it looks I mistakenly implemented ;)
I'll trust you then. Make it a standalone commit though please.


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

https://reviews.llvm.org/D86018