RFR: 8308994: C2: Re-implement experimental post loop vectorization [v2]

[Pengfei Li]

On Fri, 23 Jun 2023 10:49:50 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Pengfei Li has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address part of comments from Emanuel
>
> src/hotspot/share/opto/superword.cpp line 179:
> 
>> 177:     assert(_packset.length() == 0, "packset must be empty");
>> 178:     success = SLP_extract();
>> 179:     if (PostLoopMultiversioning) {
> 
> Could we now have an assert for `cl->is_main_loop()` at the beginning of `SuperWord::transform_loop`, and remove all checks for it in SuperWord?

Unfortunately, I just tried updating this but found assertion failures. I see `SuperWord::transform_loop()` is also called in `IdealLoopTree::policy_unroll_slp_analysis()` which can pass a normal loop (the loop before iteration-split). I assume only main loops require unrolling analysis and don't understand why it could be a normal loop. Maybe that's bad code and we need refactor C2's unrolling analysis first.

> src/hotspot/share/opto/superword.cpp line 632:
> 
>> 630:       cl->set_slp_pack_count(_packset.length());
>> 631:     }
>> 632:   } else {
> 
> Again: Could we now have an assert for `cl->is_main_loop()` at the beginning of `SuperWord::SLP_extract`, and remove all checks for it in SuperWord?

Ok to do it here as `do_optimization` is false in the unrolling analysis phase. I've updated the code in commit 2.

> src/hotspot/share/opto/superword.hpp line 251:
> 
>> 249:   int count_size(int size) {
>> 250:     return _stats[exact_log2(size)];
>> 251:   }
> 
> Add assert from `record_size`?

Done, thanks!

> src/hotspot/share/opto/superword.hpp line 666:
> 
>> 664:   IdealLoopTree*  lpt() const   { return _lpt; }
>> 665:   PhiNode* iv() const {
>> 666:     return _slp ? _slp->iv() : _lpt->_head->as_CountedLoop()->phi()->as_Phi();
> 
> I'd suggest either cache it directly from `_lpt->_head->as_CountedLoop()->phi()->as_Phi()`, or just query it directly. Reduce dependence on `_slp`.

Good catch! What do you think of getting rid of `_slp` completely in `SWPointer` refactoring?

> src/hotspot/share/opto/superword.hpp line 669:
> 
>> 667:   }
>> 668: 
>> 669:   void init();
> 
> This is just a helper function for the constructors, right? Maybe move it closer to them?

Done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250443717
PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250447018
PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250450494
PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250452509
PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250452658