llvm / llvm/llvm-project

[KnownFPClass] Encoding NaN sign in the `KnownFPClass` class mask instead of `SignBit`.

Open
#217,072 0 comments 0 reactions 0 assignees View on GitHub
floating-point llvm:support
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

Currently, `KnownFPClass` stores an `FPClassTest` together with a separate `std::optional SignBit`. The separate `SignBit` is needed because `FPClassTest` does not distinguish the signbit of qNaN/sNaN.

https://github.com/llvm/llvm-project/blob/77adee27440ec76263560a7673dfb6acfa2f2ad6/llvm/include/llvm/Support/KnownFPClass.h#L27-L34

My proposal is to change the representation of `KnownFPClass` so it encodes these:
```
kfcNegQNan
kfcNegSNan
kfcNegInf
kfcNegNormal
kfcNegSubnormal
kfcNegZero
kfcPosZero
kfcPosSubnormal
kfcPosNormal
kfcPosInf
kfcPosSNan
kfcPosQNan

kfcSNan = kfcNegSNan | kfcPosSNan
kfcQNan = kfcNegQNan | kfcPosQNan
kfcPosNan = kfcPosSNan | kfcPosQNan
kfcNegNan = kfcNegSNan | kfcNegQNan
kfcNan = kfcSNan | kfcQNan

kfcNegative = kfcNegInf | kfcNegNormal | kfcNegSubnormal | kfcNegZero
kfcPositive = kfcPosInf | kfcPosNormal | kfcPosSubnormal | kfcPosZero
kfcNegSignBit = kfcNegNan | kfcNegative
kfcPosSignBit = kfcPosNan | kfcPositive
```

This would allow `KnownFPClass` to be represented by a single bitmask instead of the bitmask and `optional` currently used. This would require a separate bitmask type for `KnownFPClass` as its bit layout differs from `FPClassTest`.

The main benefit is that this removes the separate `SignBit` state and the logic needed to keep it synchronized with the class mask. This also reduces the `sizeof(KnownFPClass)` from 8 bytes to 4 bytes on x86-64.

An additional minor benefit is being able to represent that a value can be `kfcNegSNan | kfcPosQNan` but never `kfcNegQNan | kfcPosSNan`. Note that `kfcPositive`/`kfcNegative` would still only represent positive/negative values that are not NaN. `kfcPosSignBit`/`kfcNegSignBit` can be used to test the signbit including NaN.

One small implementation detail to decide on is if the NaNs should stay together in the enum (negNaN, posNaN, negative, positive) similar to FPClassTest, or if they should follow ieee-total-ordering (negNaN, negative, positive, posNaN).

***

Pros:
- `sizeof(KnownFPClass)` is reduced from 8 bytes to 4 bytes on x86-64.
- Simplifies NaN signbit logic.
- Allows representing sNaN and qNaN with different signs.
- No change to `llvm.is.fpclass` and `FPClassTest` can remain unchanged.

Cons:
- Conversion logic is needed when converting between `KnownFPClass` and `FPClassTest`.
- Requires a new separate bitmask type for `KnownFPClass`.
- We may have to rename `fcPosZero` to `fctPosZero` and etc if we think that is necessary to reduce confusion between `KnownFPClass` and `FPClassTest`.
- Removes "invalid" states such as `{fcPosNormal, true}`.

As a separate future possibility, this may pave the way for splitting `kfcPosNormal` into `kfcPosNormalSmallerOne | kfcPosOne | kfcPosLargerOne` as discussed here https://github.com/llvm/llvm-project/issues/190809, since the bit layouts of `KnownFPClass` and `FPClassTest` would no longer need to remain identical.

***

# Progress tracking

1. Replace direct uses of the SignBit field with getters/setters: https://github.com/llvm/llvm-project/pull/218514
2. Replace direct uses of the KnownFPClasses field with getters/setters: https://github.com/llvm/llvm-project/pull/220315
3. Replace KnownFPClasses and SignBit with a single KnownFPMask: https://github.com/llvm/llvm-project/pull/220606

Contributor guide

Open the contributing guide

Research direction

Start with llvm/include/llvm/Support/KnownFPClass.h and review the three progress-tracking pull requests linked in the issue. Understand the proposed KnownFPMask representation, the required conversions with FPClassTest, and the unresolved NaN enum ordering choice. Done means the tracked migration is complete and the representation consistently replaces KnownFPClasses and SignBit.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.