RFR: 8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed [v5]

[Roberto Castañeda Lozano]

On Fri, 16 May 2025 13:06:25 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> > Note that I am not necessarily suggesting disabling "late" elimination of allocations at macro expansion. But it would be good, in light of the above findings, to find actual cases where the seemingly simpler alternative of removing dead allocations early is not sufficient for correctness, to motivate the more complex approach proposed in this PR.
> 
> What happens with this bug is that a Phi created sometime after parsing inherits the type of the projection out of the Initialize which is wrong. No issue happens until the allocation is removed though. Only having allocations be removed early one shortens the window where bad things (a new Phi) can happen. But bad things could still happen. After all, we do some loop opts to help EA so maybe a similar issue could happen there. Or maybe, down the road, someone will change the way we do loop opts during EA because it helps EA and the bug will be back but we don't necessarily notice it until it happens at a user's site.
> 
> Beyond that, you're suggesting restricting elimination of allocation. What if, down the road, someone notices that it gets in the way of some other optimization? Then that someone we'll have to reconstruct the history here.
> 
> There's a history in c2 of fixing issues that are complicated by working around them. Often what happens is that we, later on, realize that the first work around wasn't sufficient and try to pile on more arounds. I also already ran into situations where everything to perform the needed optimization is there but it's disabled for some reason in a particular case and it's unclear why.
> 
> I'm in favor of fixing issues once and for all by targeting their root cause and trying to not sacrifice performance whenever possible. Truth is we have a limited view in what people are running though a set of microbenchmarks but we can't be sure something has no performance impact only because that set of microbenchmarks doesn't regress. Beyond that, what can appear as a safer workaround today could be more trouble down the line and in the end cause more confusion and work.

Thanks for the elaborate reply, Roland. I am also in favor of fundamental fixes instead of accumulating point fixes over time. Note that I do not suggest restricting elimination of dead array allocations. I guess I am mostly surprised to learn that we do not eliminate them as early as we can, but I understand your concern that similar issues may arise even if we did. I still think it would be good to include test cases to confirm that these are not only theoretical concerns, but that should not block the progress of this PR.

I also think it would be good to investigate, separately, early elimination of dead array allocations, even after the integration of this work. Dead allocations may inhibit later optimizations so it would be good to eliminate them as early as possible anyway. One difficulty (not addressed in https://github.com/openjdk/jdk/commit/c28f81a7ef2a4f3d3cb761ea23a80c09276e7e58) is that early array elimination should still generate the nonnegative array size check code.

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

PR Comment: https://git.openjdk.org/jdk/pull/24570#issuecomment-2891102076