RFR: 8346664: C2: Optimize mask check with constant offset

[Emanuel Peter]

On Wed, 8 Jan 2025 13:37:43 GMT, Matthias Ernst <duke at openjdk.org> wrote:

>> src/hotspot/share/opto/mulnode.cpp line 2063:
>> 
>>> 2061: 
>>> 2062: // Returns a lower bound on the number of trailing zeros in expr.
>>> 2063: static jint AndIL_min_trailing_zeros(const PhaseGVN* phase, const Node* expr, BasicType bt) {
>> 
>> Is this method restricted to use in AndIL? Because it looks like it is doing something more generic: trying to figure out a lower bound on the trailing zeros of an expression.
>> 
>> If that is the case: Why not put it in `Node::get_trailing_zeros_lower_bound(phase, bt)`, so it can be used elsewhere too?
>
> I would argue that while this might be incidentally reusable outside of the scope of "And" nodes, as long as there is no actual demand to reuse this, I would rather not add it to the rather prominent Node class to avoid api bloat.
> 
> Iff the notion of "is known to be a multiple of a certain power of two" is really of general interest, I would expect it to become part of `TypeInteger`.

Sure, just leave it where it is for now. I'm ok with it.

>> test/hotspot/jtreg/compiler/c2/irTests/TestShiftAndMask.java line 221:
>> 
>>> 219:         }
>>> 220:     }
>>> 221: 
>> 
>> Nice to see that you have some examples here!
>> 
>> I think it would be great to have some more though. The divil hides in the details. In the edge cases usually.
>> 
>> You currently have patterns like this:
>> `(j + ((i + c1) << c2)) & c3;`
>> What if you generate the constants `c1, c2, c3` randomly:
>> `public static final int C1 = random.nextInt()` (or some other random distribution that makes more sense).
>> Then the compiler will see them as constants (because final), and attempt constant folding.
>> 
>> You can then do result verification: You create a method copy that you restrict to the interpreter, and the other copy can be compiled. Then you test the method with all sorts of random inputs for `i, j`, and verify the results of the two methods (compiled vs interpreted).
>> 
>> Maybe you can add some more patterns as well, just to have a better test coverage.
>> 
>> Does that make sense?
>
> I believe I understand the intent, and I've now randomized all constant masks / shifts / consts in this file. But just to make sure: IIUC the tests are only compiled once per invocation, there is no way I can tell the framework to "C2 compile this x times with different random constants". I.e. I can `make test` this a hundred times locally, but I cannot create large coverage via the framework, right?
> 
> Also not quite sure I understand the verification proposal. How would that be different from the current comparisons `if (result != expected simplified form)` ? Now if the framework supported an automatic comparison of compiled vs interpreted invocation, that would be nice.

Yeah, creating large coverage with a single run under the IR framework is not currently possible I think.

Generally, there are other tricks to get "changing constants", see what I did with `setConstant` and `int_con` in this test:
`test/hotspot/jtreg/compiler/loopopts/superword/TestAlignVectorFuzzer.java`

But the tests are rerun a lot anyway, so that is not super necessary.

I am working on a Template framework that makes using random constants much easier, and also generating multiple methods where only the constants differ. That should make things a little easier.

I suppose that works: `if (result != expected simplified form)`
Though only for cases where we have a valid simplification. If you also want to test the cases that have a very similar pattern, but should not accidentally wrongly optimize, then you would have to do the compiled/interpreted comparison.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1907431017
PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1907428608