RFR: 8333684: C2 SuperWord: multiple smaller refactorings in preparation for JDK-8332163 [v2]

[Emanuel Peter]

On Wed, 12 Jun 2024 11:32:54 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Apply suggestions from code review
>>   
>>   Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>
> src/hotspot/share/opto/superword.cpp line 1636:
> 
>> 1634: }
>> 1635: 
>> 1636: VTransformBoolTest PackSet::get_bool_test(const Node_List* bool_pack) const {
> 
> If there is a `Pack` class at some point, this should also go in there since this is actually querying the pack and not the packset.

Yes, we can move that later, it will be a quick rename without code moving ;)

> src/hotspot/share/opto/superword.cpp line 1637:
> 
>> 1635: 
>> 1636: VTransformBoolTest PackSet::get_bool_test(const Node_List* bool_pack) const {
>> 1637:   BoolNode* bol0 = bool_pack->at(0)->as_Bool();
> 
> Not sure if the `0` postfix is necessary since you do not use any other nodes of the pack where it is necessary to distinguish different nodes in the pack.

I would like to keep it. Yes, there is use of the other nodes in the `ASSERT` block below.

> src/hotspot/share/opto/superword.cpp line 2136:
> 
>> 2134: bool SuperWord::apply(Node_List& memops_schedule) {
>> 2135:   CountedLoopNode* cl = lpt()->_head->as_CountedLoop();
>> 2136:   phase()->C->print_method(PHASE_AUTO_VECTORIZATION1_BEFORE_APPLY, 4, cl);
> 
> You could store `phase()->C` into a local variable `C` and use that one.

Sure

> src/hotspot/share/opto/vectornode.cpp line 929:
> 
>> 927: bool VectorNode::is_scalar_unary_op_with_equal_input_and_output_types(int opc) {
>> 928:   switch (opc) {
>> 929:   case Op_SqrtF:
> 
> Our style guide does not really says anything about indentation of switches. I would have suggested, though, to indent it:
> 
> switch (opc) {
>   case Op_SqrtF:
>   ....
> }

The rest of the file does it the way I have it, so I think I'll keep it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636353521
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636354796
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636356448
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636357496