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

[Jan Vesely via Phabricator via llvm-commits]

jvesely added a comment.

In D78220#1986237 <https://reviews.llvm.org/D78220#1986237>, @fhahn wrote:

> In D78220#1985031 <https://reviews.llvm.org/D78220#1985031>, @jvesely wrote:
>
> > In D78220#1984311 <https://reviews.llvm.org/D78220#1984311>, @nikic wrote:
> >
> > > I would strongly recommend to implement this as a separate type (FloatRange for example), rather than combining it into ConstantRange. There's a couple reasons for that, the main ones would be a) float ranges have significantly different semantics, in particular the fact that they are inclusive and b) ConstantRange is a size critical structure, because it is used in value lattice elements. As implement, this change is going to skyrocket memory consumption during SCCP.
> >
>
>
> +1, I think there is a strong case for not increasing the size here. We should rather try to reduce the size of the existing ConstantRange (using the 2 APInts means we store the same bit width twice :()
>
> > The point of having one combined range is to allow simplification of logic for users. you don't have to check for the type just do the same operations `union/intersect/binop/contains/..`.
> >  I'm not sure how splitting it to a separate class would address the memory concerns. Tracking fp ranges will require two APFloats irrespective of the structure.
> >  If keeping both `APFloat` and `APint` bounds is a problem I can put them in a union since they are used exclusively and an instance never switches between float and int.
>
> 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.
>
> When using the FP version of the constant range in ValueLattice, that would allow us to keep the size the same, if no extra bits (like canBeNaN) are added.


OK, I'll take a stab at separating the backend, but I don't see a good way to reduce memory usage.

the current situation:
 `sizeof(APInt) == 16` and there are two of them so `sizeof(ConstantRange) == 32`, `ValueLattice` includes `ConstantRange` as member + extra tags givining it the size of 33 (aligned to 40).

possible implementations of fp tracking:
a) using unions for Lower/LowerFP, Upper/UpperFP:
`sizeof(APFloat) == 32` x2 + 2 for tags => `sizeof(ConstantRange) == 66` (aligned up to 72). `ValueLattice` includes `ConstantRange` as member bringing the size to 73 (aligned to 80).

b) union backend:
`sizeof (ConstantRangeBackendInt) == 32`, `sizeof(ConstantRangeBackendFP) == 65` (2x APFloat + bool NaN, aligned up to 72), `sizeof ConstantRange == 72)`. `ValueLattice` includes `ConstantRange` as member bring s the size to 73 (aligned to 80)

c) separate classes for int and float ranges:
`sizeof(ConstantRangeInt) == 32`, `sizeof(ConstantRangeFP) == 65` (aligns up to 72). `ValueLattice` includes both variants in a union bringing the size to 73 (aligned to 80).

The memory overhead is 2x, which is consistent with tracking more information.
As long as we have one flag, adding another another is free because of the alignment. (I used 8B alignment, but 4B align is not going to change the sizes much).

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

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.

> It might be that there is no impact on compile-time/memory usage overall with the current version (using a union), but it would be good to collect data.

Is there a test suite that I can use to collect this? Are there any integer heavy/only use cases that would only suffer overhead without any benefit to fp range tracking?

>>> It would also be nice to have some general context on where you plan to use this.
>> 
>> Other than the usual uses of range propagation, floating point ranges can be used to safely apply fast-math optimizations if operands are known to not require special handling
>>  There's already rudimentary support for NaN tracking, using ranges allows to safely apply `nosignedzeros`, `noinfs`, `nonans`, `denorms are zero` optimizations.
> 
> FWIW I think it might be good to bring up the NaN tracking & co in separate patches.

NaNs can be created as a result of normal operations (like Inf - Inf, or 0/0) so detecting NaN producing operations is required to track regular ranges. storing it in a flag is a minimal change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78220