RFR: 8327381 Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value [v4]
Jasmine Karthikeyan
jkarthikeyan at openjdk.org
Tue Mar 19 05:11:21 UTC 2024
On Wed, 13 Mar 2024 02:05:39 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:
>> This PR resolves [JDK-8327381](https://bugs.openjdk.org/browse/JDK-8327381)
>>
>> Currently the transformations for expressions with patterns `((x & m) u<= m)` or `((m & x) u<= m)` to `true` is in `BoolNode::Ideal` function with a new constant node of value `1` created. However, this is technically a type-improving (reduction in range) transformation that's better suited in `BoolNode::Value` function.
>>
>> New unit test `test/hotspot/jtreg/compiler/c2/TestBoolNodeGvn.java` asserting on IR nodes and correctness of this transformation is added and passing.
>
> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
>
> update the package name for tests
Ah, I had assumed the transformation was valid beforehand, since it had existed for a while 😅 The issue only impacts `-1`, right? Since the comparison should succeed for both `m >=0` and `m < -1`. I think it would be good to address it in this patch, as it's refactoring the existing code.
[The original patch](https://github.com/openjdk/jdk/commit/415eda1274fce4ddc78dd2221abe2ce61f7ab7f2) seems to primarily test with `array.length` as the `m` value, so the value set was nonnegative. I think we can limit the `((x & m) u< m + 1)` transform to cases where `m` is known to be nonnegative and maintain the intent behind the transform. Something like:
-} else if (_test._test == BoolTest::lt && cmp2->Opcode() == Op_AddI && cmp2->in(2)->find_int_con(0) == 1) {
+} else if (_test._test == BoolTest::lt && cmp2->Opcode() == Op_AddI && cmp2->in(2)->find_int_con(0) == 1 && phase->type(cmp2->in(1))->is_int()->_lo >= 0) {
With the IR test being modified accordingly. It'd also be good to write an IR test method that verifies that the transform doesn't take place if `m` doesn't succeed the `_lo >= 0` test.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/18198#issuecomment-2005774049
More information about the hotspot-compiler-dev
mailing list