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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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.

> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78220