RFR: 8335444: Generalize implementation of AndNode mul_ring [v3]

[Christian Hagedorn]

On Wed, 7 Aug 2024 01:20:09 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:

>> Hi all,
>> I've written this patch which improves type calculation for bitwise-and functions. Previously, the only cases that were considered were if one of the inputs to the node were a positive constant. I've generalized this behavior, as well as added a case to better estimate the result for arbitrary ranges. Since these are very common patterns to see, this can help propagate more precise types throughout the ideal graph for a large number of methods, making other optimizations and analyses stronger. I was interested in where this patch improves types, so I ran CTW for `java_base` and `java_base_2` and printed out the differences in this gist [here](https://gist.github.com/jaskarth/b45260d81ab621656f4a55cc51cf5292). While I don't think it's particularly complicated I've also added some discussion of the mathematics below, mostly because I thought it was interesting to work through :)
>> 
>> This patch passes tier1-3 testing on my linux x64 machine. Thoughts and reviews would be very appreciated!
>
> Jasmine Karthikeyan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Check IR before macro expansion

Interesting findings! I like that you unified the integer and long case with a template. A few comments.

src/hotspot/share/opto/mulnode.cpp line 604:

> 602: 
> 603:   // If both are constants, we can calculate a precise result.
> 604:   if(r0->is_con() && r1->is_con()) {

Suggestion:

  if (r0->is_con() && r1->is_con()) {

src/hotspot/share/opto/mulnode.cpp line 611:

> 609:   if (r0->_lo >= 0 && r1->_lo >= 0) {
> 610:     return IntegerType::make(0, MIN2(r0->_hi, r1->_hi), widen);
> 611:   }

Since you've already worked out the math in the PR comment, do you also want to add it here to the different cases? It could help to support the correctness of the code.

src/hotspot/share/opto/mulnode.cpp line 626:

> 624: 
> 625:   assert(r0->_lo < 0 && r1->_lo < 0, "positive ranges should already be handled!");
> 626:   static_assert(std::is_signed<NativeType>::value, "native type of IntegerType must be signed!");

Maybe you want to have that static assert on method entry already.

src/hotspot/share/opto/mulnode.cpp line 635:

> 633:   // Since count_leading_zeros is undefined at 0 (~(-1)) the number of digits in the native type can be used instead,
> 634:   // as it returns 31 and 63 for signed integers and longs respectively.
> 635:   int shift_bits = sel_val == 0 ? std::numeric_limits<NativeType>::digits : count_leading_zeros(sel_val) - 1;

`sel_val` can only be 0 if `r0->_lo` and `r1->_lo` are both -1. While I think it's correct how you handle the case here, wouldn't it be simpler/more readable if you handle this case separately by setting -1 as lower bound directly instead of using "`min >> #digits`"?

-------------

PR Review: https://git.openjdk.org/jdk/pull/20066#pullrequestreview-2245217606
PR Review Comment: https://git.openjdk.org/jdk/pull/20066#discussion_r1721586313
PR Review Comment: https://git.openjdk.org/jdk/pull/20066#discussion_r1721632659
PR Review Comment: https://git.openjdk.org/jdk/pull/20066#discussion_r1721636891
PR Review Comment: https://git.openjdk.org/jdk/pull/20066#discussion_r1721685114