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

Wed Jun 12 11:57:26 UTC 2024

On Thu, 6 Jun 2024 07:31:53 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> In preparation for https://github.com/openjdk/jdk/pull/19261, I made some smaller refactorings / moving code around:
> 
> - `SuperWord::same_input` -> `PackSet::isa_unique_input_or_null`
> - Rename print-method tags: `SUPERWORD1_BEFORE_SCHEDULE` -> `AUTO_VECTORIZATION1_BEFORE_APPLY` etc.
> - Refactored `SuperWord::schedule / output` -> `SuperWord::schedule_and_apply`:
>   - Reorganize so that we can separate out all methods that change the C2 graph into `SuperWord::apply`.
>   - Move all `phase()->C->print_method` to `SuperWord::apply`.
>   - Rename `SuperWord::schedule_reorder_memops` -> `SuperWord::apply_memops_reordering_with_schedule`.
>   - Rename `SuperWord::output` -> `SuperWord::apply_vectorization`.
> - Move `SuperWord::vectors_should_be_aligned` -> `VLoop::vectors_should_be_aligned`.
> - Move `SuperWord::requires_long_to_int_conversion` -> `VectorNode::is_scalar_op_that_returns_int_but_vector_op_returns_long`, and move comments.
> - Move `VectorNode::can_transform_shift_op` -> `VectorNode::can_use_RShiftI_instead_of_URShiftI`, and move comments.
> - Extract out `PackSet::get_bool_test` from `SuperWord::output / apply_vectorization`.
> - Extract opcode check to `VectorNode::is_scalar_unary_op_with_equal_input_and_output_types`.

Nice refactorings! A few comments, some only concern moved code but I guess it does not hurt to address some of that while touching the code anyway.

src/hotspot/share/opto/superword.cpp line 1625:

> 1623: 
> 1624: // If the j-th input for all nodes in the pack is the same unique input: return it, else nullptr.
> 1625: Node* PackSet::isa_unique_input_or_null(const Node_List* pack, int j) const {

Two things here:
1. You do not seem to be using the actual unique input at the call sites and only care about whether there is a unique input or not (all checks are against `nullptr`).
2.  I'm not sure if "unique" could be misleading as in "only once" in the entire pack. 

To address both, I suggest to change this to:
`bool PackSet::same_input_at_index()`

What do you think?

src/hotspot/share/opto/superword.cpp line 1627:

> 1625: Node* PackSet::isa_unique_input_or_null(const Node_List* pack, int j) const {
> 1626:   Node* p0 = pack->at(0);
> 1627:   Node* unique = p0->in(j);

Could probably be merged:
Suggestion:

  Node* unique = pack->at(0)->in(j);

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.

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.

src/hotspot/share/opto/superword.cpp line 1646:

> 1644:          mask == BoolTest::lt ||
> 1645:          mask == BoolTest::le,
> 1646:          "Bool should be one of: eq,ne,ge,ge,lt,le");

Add spaces and fix typo:
Suggestion:

         "Bool should be one of: eq, ne, ge, gt, lt, le");

src/hotspot/share/opto/superword.cpp line 2100:

> 2098: //     packed scalars with vector operations.
> 2099: bool SuperWord::schedule_and_apply() {
> 2100:   if (_packset.length() == 0) {

Suggestion:

  if (_packset.is_empty()) {

src/hotspot/share/opto/superword.cpp line 2101:

> 2099: bool SuperWord::schedule_and_apply() {
> 2100:   if (_packset.length() == 0) {
> 2101:     return false; // empty packset

With the change above, the comment is no longer necessary (somehow I cannot make a suggestion here to directly apply).

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.

src/hotspot/share/opto/superword.cpp line 2363:

> 2361:         BoolTest::mask test_mask = bool_test._mask;
> 2362:         if (bool_test._is_negated) {
> 2363:            // We can cancle out the negation by swapping the blend inputs.

Suggestion:

           // We can cancel out the negation by swapping the blend inputs.

src/hotspot/share/opto/vectorization.hpp line 132:

> 130:   int estimated_node_count()  const { return (int)(1.10 * phase()->C->unique()); };
> 131: 
> 132:   // should we align vector memory references on this platform?

Suggestion:

  // Should we align vector memory references on this platform?

src/hotspot/share/opto/vectorization.hpp line 1329:

> 1327:   const BoolTest::mask _mask;
> 1328:   const bool _is_negated;
> 1329:   VTransformBoolTest(const BoolTest::mask mask, bool is_negated) :

Maybe add a new line to better separate from the constructor:
Suggestion:

  VTransformBoolTest(const BoolTest::mask mask, bool is_negated) :

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:
  ....
}

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

PR Review: https://git.openjdk.org/jdk/pull/19573#pullrequestreview-2112470864
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636266162
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636267319
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636305416
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636307566
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636281109
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636311391
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636312293
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636314603
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636323064
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636319858
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636320853
PR Review Comment: https://git.openjdk.org/jdk/pull/19573#discussion_r1636325763