[PATCH] D78220: IR/ConstantRange: Add support for FP ranges

[Jan Vesely via Phabricator via llvm-commits]

jvesely added a comment.

In D78220#1987377 <https://reviews.llvm.org/D78220#1987377>, @nikic wrote:

> In D78220#1987284 <https://reviews.llvm.org/D78220#1987284>, @jvesely wrote:
>
> > Unless we use pointers and an extra level of indirection the end sizes for users like ValueLattice end up the same irrespective of where the union is (lower+upper vs. backend vs. user).
>
>
> I'd expect that this is indeed what we want to do. Given that the float range is just so much larger, we'll want to allocate it out of line.


wouldn't the indirection needlessly penalize fp oriented targets? GPUs often use runtime compilers so performance here is more important than memory usage (gpu kernels tend to be smaller than offline compiled programs).

> 
> 
>> If memory usage is a concern, dropping ConstantRange from ValueLattice structure (so it's not taking up space for `unknown`/`overdefined` tags) might be a better target.
> 
> That's also a possibility (LVI for example uses a separate set for overdefined values to reduce memory usage), but fairly orthogonal. Even if ConstantRange is allocated out of line as well, we'll still want to be able to allocate the smaller integer only structure.
> 
> In any case, it's really not just a matter of memory usage, but also semantics. Integer and float constant ranges cannot interact in any meaningful way, you can't take the union of an integer and float range for example. Your patch already asserts that range types match in many places. It is better if these invariants get enforced more robustly by the type system.
> 
> In D78220#1986237 <https://reviews.llvm.org/D78220#1986237>, @fhahn wrote:
> 
>> Even with a union, you still need extra space for the bits to distinguish between FP/integers (and other FP related flags, like `canBeNaN`. It should be possible to achieve the same level of convenience to this patch, but separating integer and FP ranges into separate classes. E.g. add ConstantRangeIntStorage & ConstantRangeFloatStorage which use APInt/APFloat respectively. The shared logic can be implemented in a ConstantRangeCommon template, which can be instantiated with either ConstantRangeIntStorage/ConstantRangeFPStorage. ConstantRange would turn into an instantiation of ConstantRangeCommon<ConstantRangeIntStorage>. Of course that would require updating code to make use of the FP version of ConstantRange. That might be a benefit actually, because there probably are various places that assume ConstantRange only contains integers.
> 
> 
> I expect that there is very little logic that can be meaningfully shared between these -- it's mostly a matter of some API names being the same, while the implementation is different. I wouldn't try too many gymnastics to share anything between them.

The logic is separate as most operations are either int or float with limited number of transitions (fptoint/inttofp). The point is to have a single instance that can handle both. the user should be able to do `getFull(Type*)/getEmpty(Type*)/isSingleElement()/getSingleElementConstant(...)/getSatisfyingRange(CmpInst).contains(item1.getRange())` without having to duplicate the logic for integer and floating point types.
Separate classes impose extra complexity on every user that wants to track floating point values. it's not just handling of storage in a union (which at least for ValueLattice is mostly in place), but every single function that currently returns a `ConstantRange` will need to be duplicated. this leads to error-prone code structure.

The current version of the patches intentionally keeps things separate for reviewing, but the plan is to unify most of the duplicated code paths introduced in D78224 <https://reviews.llvm.org/D78224>. Even in D78224 <https://reviews.llvm.org/D78224> the codepaths that only deal with ranges are unified, the separation is mainly for range creation or extracting single values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78220