RFR: 8332163: C2 SuperWord: refactor PacksetGraph and SuperWord::output into VTransformGraph [v11]

[Emanuel Peter]

On Thu, 4 Jul 2024 09:31:58 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   rm newline
>
> Looks already very good now, thanks for the updates and the offline discussion! I like having the new files which better separates the code.
> 
> I only have a few other mostly minor comments.

@chhagedorn thanks for another round of reviews. I think I addressed all your comments :)

> src/hotspot/share/opto/superwordVTransformBuilder.hpp line 48:
> 
>> 46:       _vtransform(vtransform)
>> 47:   {
>> 48:     assert(_vtransform.is_empty(), "constructor is passed an empty vtransform");
> 
> How about naming this `has_graph()`?

Good idea.

> src/hotspot/share/opto/vtransform.cpp line 253:
> 
>> 251: }
>> 252: 
>> 253: VTransformApplyResult VTransformBoolVectorNode::apply(const VLoopAnalyzer& vloop_analyzer, const GrowableArray<Node*>& vnode_idx_to_transformed_node) const {
> 
> Maybe wrap this long line:
> Suggestion:
> 
> VTransformApplyResult VTransformBoolVectorNode::apply(const VLoopAnalyzer& vloop_analyzer, 
>                                                       const GrowableArray<Node*>& vnode_idx_to_transformed_node) const {
> 
> Same at other apply methods.

Good idea, applied it for all `apply` methods

> src/hotspot/share/opto/vtransform.hpp line 421:
> 
>> 419:   }
>> 420: 
>> 421:   const GrowableArray<Node*> nodes() const { return _nodes; }
> 
> Shouldn't this return a const reference?

Oh wow, that is a good catch!
I guess under the hood this did not make a difference, since `GrowableArray` does shallow copy, and while we use the copy we would not modify the original. But still, this should be a reference ;)

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

PR Comment: https://git.openjdk.org/jdk/pull/19719#issuecomment-2208640750
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1665492126
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1665498298
PR Review Comment: https://git.openjdk.org/jdk/pull/19719#discussion_r1665500090