RFR: 8346664: C2: Optimize mask check with constant offset [v4]
Emanuel Peter
epeter at openjdk.org
Thu Jan 23 12:33:57 UTC 2025
On Thu, 23 Jan 2025 10:33:27 GMT, Matthias Ernst <duke at openjdk.org> wrote:
>Respectfully, my time is also not infinite.
Absolutely, let's come to a reasonable compromise then. And I'm very thankful for the time you have already put into this!
> This test suite was acceptable in 2022 with constants 2 and 3
I would not have accepted it back then, and the test coverage is not sufficient in my view. We now have reasonable confidence that the code before your patch is probably correct, because of fuzzer tests that have run over it for 2+ years. But now you are refactoring the code, and that lowers my confidence, even if we are diligently reviewing the code. Other reviewers may accept a lower coverage, I don't.
Attempt at Compromise:
I'm ok leaving the `Generators` idea to a follow-up RFE. I can file one and find someone to do that afterward if you don't want to. Let me know what you decide on this.
Still I would like to see a fully randomized example for the most general case, one for `int` and one for `long`:
`(j + (i << con1) + con2) & con3`
Where the constants are totally free in the `int` and `long` range.
If it already exists just point me to it ;)
Once you add that test, I'll be ok with it, and we can integrate.
I know this took long here. One reason was the OCA-verify, another the Christmas break, and yet another that I am reviewing 10+ PRs in parallel. I look at one PR, see a few things, comment on them. I may not have found everything at once, so it is an iterative process. And it may look like this optimization is "just a small case", but these optimizations often end up on quite important code paths, for bounds or alignment checks - they really should not be wrong. Imagine the original optimization actually has a bug, and now through your extension it may apply to many more cases than before. That is why I take test coverage quite serious.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1926903752
More information about the hotspot-compiler-dev
mailing list