Integrated: 8308917: C2 SuperWord::output: assert before bailout with CountedLoopReserveKit

[Emanuel Peter]

On Fri, 26 May 2023 04:43:20 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> In SuperWord::output we create a CountedLoopReserveKit, so that we can reverse edits to the loop, in case something goes wrong. As far as I understand all of these conditions should never occur, prior condition checking in SuperWord should have already verified that. We should at least add asserts so that we can catch such failures and fix them, and do not just silently bail out of SuperWord (reverse the graph to before SuperWord and continue compilation).
> 
> `DoReserveCopyInSuperWord` enables `do_reserve_copy()`. It is a product flag and default true. If it is disabled, and there is such a failure we just hit a `ShouldNotReachHere()`.
> 
> There was one occurance I could not assert for: `vmask = create_post_loop_vmask();`. Read more below, there is actually a but there.
> 
> **Testing**
> 
> TODO testing up to tier6 plus stress testing.
> (it already passed tier3 and stress testing)
> 
> **Discussion**
> 
> Do we really want to keep the `DoReserveCopyInSuperWord` flag (product, always true), which enables the use of `CountedLoopReserveKit`? It means that we always duplicate the loop (and the loops can be rather large because they were unrolled before SuperWord). It seems a bit of an edge case to want to bail out of SuperWord, but not of the whole compilation.
> We can later decide if it makes sense to clone the whole loop via CountedLoopReserveKit (the loops can be large!), or if we should just have a regular compilation bailout instead (could simplify the code and reduce overhead of loop cloning).
> 
> Plus: it seems the checks and bailouts are very selectively applied. I don't see why we would nullptr check some "vector_opd" but not all of them. So if we decide to keep it, we should probably apply it more consistently.
> 
> What do you think?
> 
> ------
> 
> **Bug: bad combination of -XX:+PostLoopMultiversioning -XX:-DoReserveCopyInSuperWord**
> 
> I filed it here: [JDK-8308949](https://bugs.openjdk.org/browse/JDK-8308949)
> 
> `PostLoopMultiversioning` unrolls the post-loop with the use of a vmask. Read more about post-loop vectorization here https://github.com/openjdk/jdk/pull/6828. But in `create_post_loop_vmask` we have some conditions which have to hold, and if they fail we get a `nullptr`, and bail out of SuperWord, via `CountedLoopReserveKit`.
> 
> But if we turn off `DoReserveCopyInSuperWord`, this is not cought, and we hit an assert.
> 
> Generally, this looks a bit unclean, what we have now: we should do the checks of `create_post_loop_vmask` before `SuperWord::output`,...

This pull request has now been integrated.

Changeset: 25b98030
Author:    Emanuel Peter <epeter at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/25b98030569d863e605f398d5f97211008c58ca3
Stats:     5 lines in 1 file changed: 5 ins; 0 del; 0 mod

8308917: C2 SuperWord::output: assert before bailout with CountedLoopReserveKit

Reviewed-by: kvn, thartmann

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

PR: https://git.openjdk.org/jdk/pull/14168