RFR: 8346664: C2: Optimize mask check with constant offset [v3]

[Emanuel Peter]

On Wed, 29 Jan 2025 17:42:18 GMT, Matthias Ernst <duke at openjdk.org> wrote:

>> @mernst-github I'm sorry you feel that this change has taken too long to review. It certainly was delayed for some unfortunate reasons (OCA check, Christmas holidays). However, I think you are very mistaken in your comments about Emanuel's interest in your patch and commitment to getting it integrated. When he said he was juggling many other reviews that is the honest truth. His commitment to this project and the hard work that results is very much appreciated by other developers and reviewers. What Emanuel omitted to add was that he is at the same time working on committing his own changes to the compiler code base.
>> 
>> Far from kicking the can down the road Emanuel has actually asked you to make successive improvements to both the code and the tests that go with it. The rationale for posting such requests is something that many newer committers to this project often find difficult to understand and some think it is down to something personal but I can assure you it is not and does not mean your contribution is undervalued. The project sets very high standards for anything that gets committed and expects reviewers to ensure those standards are met. We all want the code base to be (and remain) as simple and clear as possible and the test base to provide strong guarantees that we will find errors now in newly committed code and later should the code base get updated. That is a difficult hurdle for some contributors to jump over but it is not one we are willing to lower.
>
> @adinn @eme64 
> 
> I appreciate you responding on this. Let me concede upfront that my message assigned intent; that was not adequate and I apologize for that. I’m sure intentions are good, but execution has been somewhat frustrating in this case.
> 
> We may need to agree to disagree on a few of points here. Let me try and make a few constructive suggestions, and as usual they are all about communication:
> 
> * Your remark about high standards and newcomers misses the mark. As you can see from the PR conversation, all the tests I added adhered to the same standards I found in the code base. That “hurdle” you talk about that I allegedly have trouble accepting is one you had already lowered. Please acknowledge that I was happy to update the entire test suite as requested. So a much better wording you might want to use when starting the review is “Our test coverage for MulNode is currently not adequate and we are uncomfortable making any changes there. I will need to ask you to rewrite the tests first.” Instead of telling me that my patterns are lacking. That’s a challenge I can accept to take or leave.
> * Avoid changing the goal posts. I was happy to improve the tests as suggested in https://github.com/openjdk/jdk/pull/22856#discussion_r1906599399 only to be told two weeks later to rewrite them again because now the new Generators framework had been committed.
> * Latency latency latency. More cycles = more changes = better quality.  When you schedule your reviews round-robin, and every “Feel free to ping me :)” takes O(48h) your conversation partner quickly burns out. You can also lower latency by parallelizing: if you have a more comprehensive test suite besides the unit tests you’re asking me to improve, why not kick it off already at the point where you say “the VM changes look good to me”? “The PR as is passes our CI” would be a wonderful message to send.
> * Give me the impression that you spent more than a minute with my PR. “Point me to the tests where mask_t is a range” after I’ve already mentioned “NonConstMask” in the same conversation is not encouraging. ^F is your friend.
> * Basic comms: provide some acknowledgement upfront that a) the issue is valid, b) there’s a desire to see it fixed and c) that you agree with the approach laid out in the PR. You can do that as part of the first review. After that we can talk about copyright headers and why did I forward-declare that function.
> 
> Happy to have this conversation, not interested in a shouting match, even when I...

@mernst-github Tests launched. Could take 24-48h.

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

PR Comment: https://git.openjdk.org/jdk/pull/22856#issuecomment-2623683600