RFR: 8251925: C2: RenaissanceStressTest fails with assert(!had_error): bad dominance [v2]

[Christian Hagedorn]

On Fri, 30 Oct 2020 17:13:41 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Check dominance with pre loop head instead of tail
>
> I would also suggest to run locally next jtreg command and compare number of created vector nodes to make sure your changes did not affect common cases:
> 
> `$ jtreg -testjdk:/my_jdk/ -va -javaoptions:'-server  -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+TraceNewVectors' -J-Djavatest.maxOutputSize=1000000 compiler/c2/cr6340864/ compiler/codegen/ compiler/loopopts/superword/ compiler/vectorization >new_vects.log`
> 
> `$ grep "new Vector node:" new_vects.log|wc`

@vnkozlov Thanks for your review!

> $ jtreg -testjdk:/my_jdk/ -va -javaoptions:'-server -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+TraceNewVectors' -J-Djavatest.maxOutputSize=1000000 compiler/c2/cr6340864/ compiler/codegen/ compiler/loopopts/superword/ compiler/vectorization >new_vects.log
>
> $ grep "new Vector node:" new_vects.log|wc

I ran that with the new commit - there is no difference between the fix and base without fix:
$ grep "new Vector node:" new_vects_fix.log | wc
   9439  227498 1941126
$ grep "new Vector node:" new_vects_base.log | wc
   9437  227521 1941437

> src/hotspot/share/opto/superword.cpp line 3801:
> 
>> 3799: }
>> 3800: 
>> 3801: bool SWPointer::invariant_not_dominated_by_pre_loop_end(Node* n) {
> 
> I think we should have only one invariant() method which does this additional check.
> And have separate method is_main_loop_member() where currently we check !invariant().

That's a good idea, fixed.

> src/hotspot/share/opto/superword.cpp line 3810:
> 
>> 3808:     // This happens, for example, when n_c is a CastII node that prevents data nodes to flow above the main loop and into
>> 3809:     // the pre loop. Use the cached version as the real pre loop end might not be found anymore with get_pre_loop_end().
>> 3810:     return !phase()->is_dominator(_slp->cached_pre_loop_end(), n_c);
> 
> I think it is not enough. We don't want invariant be inside pre-loop.
> Invariant should be node outside original loop (before splitting and unrolling).
> We should check that n_c dominates pre-loop head.
> What do you think?

I think that makes sense. I updated that. Tests still pass.

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

PR: https://git.openjdk.java.net/jdk/pull/954