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

[Emanuel Peter]

On Thu, 10 Apr 2025 15:15:54 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> This is a variant of 8332827. In 8332827, an array access becomes
> dependent on a range check `CastII` for another array access. When,
> after loop opts are over, that RC `CastII` was removed, the array
> access could float and an out of bound access happened. With the fix
> for 8332827, RC `CastII`s are no longer removed.
> 
> With this one what happens is that some transformations applied after
> loop opts are over widen the type of the RC `CastII`. As a result, the
> type of the RC `CastII` is no longer narrower than that of its input,
> the `CastII` is removed and the dependency is lost.
> 
> There are 2 transformations that cause this to happen:
> 
> - after loop opts are over, the type of the `CastII` nodes are widen
>   so nodes that have the same inputs but a slightly different type can
>   common.
>   
> - When pushing a `CastII` through an `Add`, if of the type both inputs
>   of the `Add`s are non constant, then we end up widening the type
>   (the resulting `Add` has a type that's wider than that of the
>   initial `CastII`).
>   
> There are already 3 types of `Cast` nodes depending on the
> optimizations that are allowed. Either the `Cast` is floating
> (`depends_only_test()` returns `true`) or pinned. Either the `Cast`
> can be removed if it no longer narrows the type of its input or
> not. We already have variants of the `CastII`:
> 
> - if the Cast can float and be removed when it doesn't narrow the type
> of its input.
> 
> - if the Cast is pinned and be removed when it doesn't narrow the type
> of its input.
> 
> - if the Cast is pinned and can't be removed when it doesn't narrow
> the type of its input.
> 
> What we need here, I think, is the 4th combination:
> 
> - if the Cast can float and can't be removed when it doesn't narrow
> the type of its input.
> 
> Anyway, things are becoming confusing with all these different
> variants named in ways that don't always help figure out what
> constraints one of them operate under. So I refactored this and that's
> the biggest part of this change. The fix consists in marking `Cast`
> nodes when their type is widen in a way that prevents them from being
> optimized out.
> 
> Tobias ran performance testing with a slightly different version of
> this change and there was no regression.

@rwestrel thanks for looking into this one!

I have not yet deeply studied the PR, but am feeling some confusion about the naming.

I think the `DependencyType` is really a good step into the right direction, it helps clean things up.

I'm wondering if we should pick either `depends_only_on_test` or `pinned`, and use it everywhere consistently. Having both around as near synonymes (antonymes?) is a bit confusing for me.

I'll look into the code more later.

src/hotspot/share/opto/castnode.cpp line 39:

> 37: const ConstraintCastNode::DependencyType ConstraintCastNode::WidenTypeDependency(true, false, "widen type dependency"); // not pinned, doesn't narrow type
> 38: const ConstraintCastNode::DependencyType ConstraintCastNode::StrongDependency(false, true, "strong dependency"); // pinned, narrows type
> 39: const ConstraintCastNode::DependencyType ConstraintCastNode::UnconditionalDependency(false, false, "unconditional dependency"); // pinned, doesn't narrow type

Is there really a good reason to have the names `Regular`, `WidenType`, `Strong` and `Unconditional`? Did we just get used to these names over time, or do they really have a good reason for existance. They just don't really mean that much to me. Calling them (non)pinned and (non)narrowing would make more sense to me.

src/hotspot/share/opto/castnode.hpp line 58:

> 56:     bool depends_only_on_test() const {
> 57:       return _depends_only_on_test;
> 58:     }

Is this synonimous to `non_pinning`? Might that be more descriptive?

src/hotspot/share/opto/castnode.hpp line 91:

> 89:   private:
> 90:     const bool _depends_only_on_test; // Does this Cast depends on its control input or is it pinned?
> 91:     const bool _narrows_type; // Does this Cast narrows the type i.e. if input type is narrower can it be removed?

I think it would be good to have a really strong definition of these two, because everything else depends on it.

I would recommend to either use `depends_only_on_test` as the "primary" word here, or else `pinned`. But then try to consistently use the chosen one everywhere. Just to avoid confusion with these near synonymes.

It may also be helpful to have an example for each of the 4 combinations, just as an illustration of your definitions.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24575#pullrequestreview-2786860650
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2055783250
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2055785871
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2055790912