RFR: 8343685: C2 SuperWord: refactor VPointer with MemPointer [v14]

[Emanuel Peter]

On Fri, 17 Jan 2025 13:41:03 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Batch 2 for Christian
>
> src/hotspot/share/opto/mempointer.hpp line 510:
> 
>> 508:     if (p1.variable() == nullptr) {
>> 509:       return (p2.variable() == nullptr) ? 0 : 1;
>> 510:     } else if (p2.variable() == nullptr) {
> 
> `else` can be removed:
> Suggestion:
> 
>     } if (p2.variable() == nullptr) {

Can do. Adjusted the formatting a little.

> src/hotspot/share/opto/superword.cpp line 98:
> 
>> 96: };
>> 97: 
>> 98: void SuperWord::unrolling_analysis(const VLoop &vloop, int &local_loop_unroll_factor) {
> 
> This method is quite complex. Is it possible to add a short summary as method comment? Can also be done in a follow-up since you have not changed much in this refactoring.

I added this:

// SuperWord unrolling analysis does:                                                                                                              
// - Determine if the loop is a candidate for auto vectorization (SuperWord).                                                                      
// - Find a good unrolling factor, to ensure full vector width utilization once we vectorize.

I will have to re-visit this once I'm doing if-conversion anyway, and then I can refactor and write better documentation. The names in this method are not great either. But Let's do that at a later point ;)

> src/hotspot/share/opto/superword.cpp line 559:
> 
>> 557: 
>> 558: // Collect all memops that could potentially be vectorized.
>> 559: void SuperWord::collect_valid_memops(GrowableArray<MemOp>& memops) {
> 
> Can be made const.

Nice catch! Done.

> src/hotspot/share/opto/superword.cpp line 2930:
> 
>> 2928: 
>> 2929:   // 1.3: base (unless base is guaranteed aw aligned)
>> 2930:   if (aw > ObjectAlignmentInBytes || is_base_native) {
> 
> You could move the definition of `is_base_native` down before this line to easier see where it's coming from.

Sure, done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21926#discussion_r1920981446
PR Review Comment: https://git.openjdk.org/jdk/pull/21926#discussion_r1920975154
PR Review Comment: https://git.openjdk.org/jdk/pull/21926#discussion_r1920977190
PR Review Comment: https://git.openjdk.org/jdk/pull/21926#discussion_r1920979171