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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D78220#1987284 <https://reviews.llvm.org/D78220#1987284>, @jvesely wrote:

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


I think there are 2 ways to stage the introduction: 1) extending ConstantRange as in this patch is overall less work in terms of patches, but will probably require more analysis on the impact and/or more convincing as ConstantRange is widely used. 2) Introducing a ConstantRangeFP first allows clients to migrate step-by-step and would probably be easier to get in incrementally and things can unified later, once any outstanding issues are addressed.

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

Not sure about specific test cases, but people commonly use CTMark (from test-suite) and/or various versions of SPEC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78220