RFR: 8349479: C2: when a Type node becomes dead, make CFG path that uses it unreachable [v2]

[Roland Westrelin]

On Fri, 14 Mar 2025 10:51:45 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> So, I think Assertion Predicates can still be useful as part of Loop Predication and Range Check Elimination. But we have many more places where `Type` nodes could die and break the graph. This patch can indeed solve this in general and I'm definitely in favor of having this patch to solve this problem and future problems with `Type` nodes once and for all.

That makes sense and I agree with that.

One benefit of predicates (including assert predicates) is that they help narrow type further. So:


for (int i = start; i < stop; i++) {
  v += array[i];
}


is transformed by predication into:


if (start <0 || start >= array.length) { trap(); }
if (stop <0 || stop >= array.length) { trap(); }
for (int i = start; i < stop; i++) {
  v += array[i];
}

which the type propagation of 8275202 analyzes as:


if (start <0 || start >= array.length) { trap(); }
if (stop <0 || stop >= array.length) { trap(); }
// start >= 0, stop >= 0 here
for (int i = start; i < stop; i++) { // i >= 0
    v += array[i];
}


Capturing `i >= 0` in the loop `Phi` or array address `CastII` or `ConvI2L` then enables better use of address modes on x86. Except, narrowing the type of the `Phi` or `CastII` expose sC2 to the exact bug this PR tries to fix: what if the loop becomes unreachable but C2 can't fold it away and the `Phi` or `CastII` end up having an out of range input?

> I guess when moving forward with this patch we should still consider how easy it is to keep data and control in sync when adding new `Type` nodes and not just rely on this patch to make things right. We might otherwise miss to remove some other dead nodes.

For the test case that I added for this bug, the issue is that some `CastII` transformations widen the types of some nodes. I suppose the way to fix this would be to restrict those transformations so widening doesn't happen in some cases. It's going to be tricky (because widening happens so mostly identical `CastII` nodes can be commoned to improve code quality) and fragile (if to preserve performance, we choose to only restrict those transformations to few targeted cases).

For 8275202, what I tried doing is that when the new pass proves a condition constant, rather than constant fold the condition, it mark the test as always failing/succeeding (so (If (Bool ...))` is transformed into `(If (Opaque4 (Bool` and the `Opaque4` captures the final result of the `Bool`. Then the `Opaque4` constant folds later. I found several issues with this:

- how late is late enough so constant folding the opaque node is safe? In the worst case (and that applies to assert predicates too), as long as a transformation can happen then the `Opaque4` shouldn't be folded. But given folding the `Opaque4` causes more transformations to happen...

- how do we prevent this mechanism from leaking elsewhere. Say the condition is in a loop. C2 considered the loop for unrolling but the loop body was too big. With the condition folded, the loop body shrinks and we could unroll. That doesn't work with the `Opaque4` construct. Do we want to modify loop body size computation to account for that?

- Another example of the same problem. The condition is a range check. The new pass finds it to be always successful so it changes the condition to be the `Opaque4` node. Now, next round of loop predication would find the condition to be subject to predication. But now the opaque node is in the way. Do we modify loop predication so it can handle those new `Opaque4` conditions? How: is the predicate a condition with an `Opaque4` as well? What about assert predicates? What about range check elimination: what should it do with the `Opaque4` conditions?

- Say the condition is the only one in the loop. Removing it would enable vectorization. How does superword deal with the condition that shouldn't be there anymore but is still there?

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

PR Comment: https://git.openjdk.org/jdk/pull/23468#issuecomment-2724728187