RFR: 8353345: C2 asserts because maskShiftAmount modifies node without deleting the hash

[Christian Hagedorn]

On Tue, 1 Apr 2025 11:51:13 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

> First delete the hash, then `set_req`. This way, we avoid changing the node (a non-`this` node) without deleting the hash. This wrong ordering is not new from [JDK-8347459](https://bugs.openjdk.org/browse/JDK-8347459), but before that, only `this` was going through this function, so it was ok. But since, it is used with other nodes, hence the need to remove the hash.
> 
> Also, not do any of that outside IGVN, but requires to register nested shifts for IGVN in parsing not to miss them later.
> 
> Thanks,
> Marc

src/hotspot/share/opto/mulnode.cpp line 981:

> 979:     }
> 980:     return maskedShift;
> 981:   }

IIUC, we are masking a shift with a too large shift amount by masking it but normally that's directly done when transforming the inner shift. However, we could also have cases where we already process an outer shift where the inner shift is not yet replaced with the masked shift amount. So, we do the inner shift transformation as part of the outer shift processing.

It seems that for the double shift optimization, we only care about the actual masked value. I reckon that the inner shift is always on the worklist somewhere and will eventually be transformed later - there is no need to do it eagerly now (if it's not on the worklist, we should update the notification code for IGVN). This suggests that we could do the following instead:

- Rename `maskShiftAmount` into `mask_and_replace_shift_amount()`.
- Introduce `mask_shift_amount()` that only calculates the masked shift amount without updating it in the graph.
- Update `mask_and_replace_shift_amount()`: First call `mask_shift_amount()` to get the masked amount. If it's different, do the graph surgery. Return the masked shift amount.
- Use `mask_and_replace_shift_amount()` everywhere where we can safely do the update, i.e. where we call the `maskShiftAmount()` with `this`.
- Use `mask_shift_amount()` where we used to call `maskShiftAmount()` with `non-this`, i.e. surgery is not implicitly safe.

Then you also do not need the `record_fo_igvn()` code below.

test/hotspot/jtreg/compiler/c2/gvn/DoubleLShiftCrashDuringIGVN.java line 35:

> 33: 
> 34: public class DoubleLShiftCrashDuringIGVN {
> 35:     public static long shift=0;

Suggestion:

    public static long shift = 0;

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24355#discussion_r2024663634
PR Review Comment: https://git.openjdk.org/jdk/pull/24355#discussion_r2024578423