RFR: 8349139: C2: Div looses dependency on condition that guarantees divisor not null in counted loop [v2]

[Roland Westrelin]

On Tue, 25 Feb 2025 08:19:59 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Hmmm, may be you are right. I think adding a comment at `PhiNode` saying that people must not rely on it being pinned at the `Region` for dependencies would be a wise move, I can't think of any reason for that besides value narrowing right now but being pinned is a property of `Phi` regardless and we should tell people not to rely on this behaviour.
>> 
>> For this bug, I think a more general fix is to try to compare the type of the `Phi` with that of the input it is going to be replaced with. If the former is not wider than the latter then we add a `CastNode`, since the cast is only about value range, not strict dependency, we can use `CarryDependency` instead of `UnconditionalDependency`. Am I right?
>
> Ah, I only just now read the comments from @merykitty and you. Oops. Hmm.
> 
> Yes it seems that the `CountedLoop` trip `phi` is special. That's maybe not great to have such implicit assumptions laying around. But not sure what would have been the better alternative.
> 
> @rwestrel 
>> It reproduces this issue and is actually a better test case because it doesn't even need StressGCM:
> 
> Are you saying this is another test? I'd really be happy if we had more tests for this case, because the current version seems fragile, since it is an almost perfect copy of a previous test that became ineffective this makes me even more nervous 😅

@eme64 the PR is ready for reviews in case you have time.

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

PR Comment: https://git.openjdk.org/jdk/pull/23617#issuecomment-2760716302