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

Thu Apr 16 02:49:30 PDT 2020

fhahn added a comment.

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.

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.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78220