RFR: 8299546: C2: MulLNode::mul_ring() wrongly returns bottom type due to casting errors with large numbers [v3]
Christian Hagedorn
chagedorn at openjdk.org
Fri Jan 20 15:34:28 UTC 2023
> The current logic in `MulLNode::mul_ring()` casts all `jlong` values of the involved type ranges of a multiplication to `double` in order to catch overflows when multiplying the two type ranges. This works fine for values in the `jlong` range that are not larger than 2<sup>53</sup> or lower than -2<sup>53</sup>. For numbers outside that range, we could experience precision errors because these numbers cannot be represented precisely due to the nature of how doubles are represented with a 52 bit mantissa. For example, the number 2<sup>53</sup> and 2<sup>53</sup> + 1 both have the same `double` representation of 2<sup>53</sup>.
>
> In `MulLNode::mul_ring()`, we could do a multiplication with a `lo` or `hi` value of a type that is larger than 2<sup>53</sup> (or smaller than -2<sup>53</sup>). In this case, we might get a different result compared to doing the same multiplication with `jlong` values (even though there is no overflow/underflow). As a result, we return `TypeLong::LONG` (bottom type) and missed an optimization opportunity (e.g. folding an `If` when using the `MulL` node in the condition etc.).
>
> This was caught by the new verification code added to CCP in [JDK-8257197](https://bugs.openjdk.org/browse/JDK-8257197) which checks that after CCP, we should not get a different type anymore when calling `Value()` on a node. In the found fuzzer testcase, we run into the precision problem described above for a `MulL` node and set the type to bottom during CCP (even though there was no actual overflow). Since the type is bottom, we do not re-add the node to the CCP worklist because the premise is that types only go from top to bottom during CCP. Afterwards, an input type of the `MulL` node is updated again in such a way that the previously imprecise `double` multiplication in `mul_ring()` is now exact (by coincidence). We then hit the "missed optimization opportunity" assert added by JDK-8257197.
>
> To fix this problem, I suggest to switch from a `jlong` - > `double` multiplication overflow check to an overflow check without casting. I've used the idea that `x = a * b` is the same as `b = x / a` (for `a != 0` and `!(a = -1 && b = MIN_VALUE)`) which is also applied in `Math.multiplyExact()`: https://github.com/openjdk/jdk/blob/66db0bb6a15310e4e60ff1e33d40e03c52c4eca8/src/java.base/share/classes/java/lang/Math.java#L1022-L1036
>
> The code of `MulLNode::mul_ring()` is almost identical to `MulINode::mul_ring()`. I've refactored that into a template class in order to share the code and simplified the overflow checking by using `MIN/MAX4` instead of using nested `if/else` statements.
>
> Thanks,
> Christian
Christian Hagedorn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
- Change algorithm to handle overflows/underflows if the cross products have the same number of overflows/underflows as suggested by Quan
- Merge branch 'master' into JDK-8299546
- review
- 8299546: C2: MulLNode::mul_ring() wrongly returns bottom type due to casting errors with large numbers
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/11907/files
- new: https://git.openjdk.org/jdk/pull/11907/files/0d438caf..0453789f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11907&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11907&range=01-02
Stats: 11310 lines in 803 files changed: 6826 ins; 1739 del; 2745 mod
Patch: https://git.openjdk.org/jdk/pull/11907.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11907/head:pull/11907
PR: https://git.openjdk.org/jdk/pull/11907
More information about the hotspot-compiler-dev
mailing list