RFR: 8354282: C2: more crashes in compiled code because of dependency on removed range check CastIIs [v3]

[Emanuel Peter]

On Wed, 12 Nov 2025 07:24:01 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - review
>>  - infinite loop in gvn fix
>>  - renaming
>
> src/hotspot/share/opto/castnode.cpp line 47:
> 
>> 45: Node* ConstraintCastNode::Identity(PhaseGVN* phase) {
>> 46:   if (!_dependency.narrows_type()) {
>> 47:     return this;
> 
> Can you please add a code comment? I don't understand it right away :/

Maybe I'm slowly starting to understand... but a code comment would still help a lot here.

We are trying to find a dominating cast that has the same or narrower type, and replace with that one.
We are only allowed to do that if we have a narrowing cast, because ...

> src/hotspot/share/opto/castnode.cpp line 277:
> 
>> 275: 
>> 276: CastIINode* CastIINode::pin_array_access_node() const {
>> 277:   assert(depends_only_on_test(), "already pinned");
> 
> Would this not be more readable?
> 
> Suggestion:
> 
>   assert(is_dependency_floating(), "already pinned");

Because it seems we are talking about floating vs pinned here. Adding yet another concept of "depending only on test" would require further explanation / definition.

> src/hotspot/share/opto/castnode.cpp line 588:
> 
>> 586: 
>> 587:     // If both inputs are not constant then, with the Cast pushed through the Add/Sub, the cast gets less precised types,
>> 588:     // and the resulting Add/Sub's type is wider than that of the Cast before pushing.
> 
> I find this long sentence a bit complicated to read. Can you reformulate and maybe break it into smaller sentences?
> It would also be good to explicitly say why that may require changing the dependency constraint.

I wonder if you renamed `widen_type_dependency` to `with_non_narrowing`, and explained that this now prevents folding away the cast if input types are narrower, etc... that would maybe be more straight forward?

I suppose your approach was to just "notify" the dependency that we have widened the type, and then the dependency manages what the implications are. But I find that approach a bit less straight forward, because we are not talking about widening the exact same cast, but a cast that has been pushed through an add/sub. Maybe you can manage to make a coherent argument though, up to you.

> src/hotspot/share/opto/castnode.cpp line 625:
> 
>> 623:   if (!phase->C->post_loop_opts_phase()) {
>> 624:     return this_type;
>> 625:   }
> 
> Honestly, I would prefer to see this "delay to post loop opts" to be done outside of `widen_type`. It would just make more sense there. What do you think?

But maybe that is a refactoring for a separate RFE, and then not really worth it.

> src/hotspot/share/opto/castnode.hpp line 53:
> 
>> 51:         _narrows_type(narrows_type),
>> 52:         _desc(desc) {
>> 53:     }
> 
> Could you make the constructor private, and only expose the 4 static fields? That way, nobody comes to the strange idea to construct one of these themselves ;)

That would probably require moving the 4 static fields into this class here.
Example:
`ConstraintCastNode::DependencyType::FloatingNarrowing`

Just an idea. Maybe you have a different solution. But a private constructor would be great for sure.

> src/hotspot/share/opto/castnode.hpp line 146:
> 
>> 144:   virtual uint ideal_reg() const = 0;
>> 145:   bool carry_dependency() const { return !_dependency.cmp(FloatingNarrowingDependency); }
>> 146:   virtual bool depends_only_on_test() const { return _dependency.floating(); }
> 
> Why not rename it to `is_dependency_floating`? That may be more helpful at the use site.

Otherwise you have to give an explanation/code comment about the concept "depending on test", and define it in terms of floating / non-floating.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517268181
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517304372
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517331973
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517345703
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517217941
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517358981