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

[Hideto Ueno via Phabricator via llvm-commits]

uenoku added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1956
     Known = Known.unionWith(R);
-    Assumed = Assumed.unionWith(Known);
   }
----------------
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 ;)


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

https://reviews.llvm.org/D86018