[Bug target/111114] New: RISC-V: False combine extend + vcond_mask when modify by vect_recog_over_widening_pattern

[Bug target/111114] New: RISC-V: False combine extend + vcond_mask when modify by vect_recog_over_widening_pattern

[lehua.ding at rivai dot ai]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111114

            Bug ID: 111114
           Summary: RISC-V: False combine extend + vcond_mask when modify
                    by vect_recog_over_widening_pattern
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: lehua.ding at rivai dot ai
  Target Milestone: ---

The vect_recog_over_widening_pattern in vect pass will change:
  v2 = EXTEND (v1)
  v3 = VEC_COND_EXPR (mask, v2, { 1, 1, ... })
to:
  v2 = VEC_COND_EXPR (mask, v1, { 1, 1, ... })
  v3 = EXTEND (v2)
when it is safe. This change looks like no gain for riscv and makes it
impossible to combine them into a MASK_EXTEND in combine pass.

https://godbolt.org/z/PWE4rzr7j

[Bug tree-optimization/111114] RISC-V: Failed combine extend + vcond_mask when modify by vect_recog_over_widening_pattern

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111114

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-08-23
     Ever confirmed|0                           |1
          Component|target                      |tree-optimization
                 CC|                            |pinskia at gcc dot gnu.org

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There is a match pattern which handles the case where the convert (cast) will
fold both sides of the vec_cond now (since r14-3337-g70c50c87273d94).

  vect__5.14_56 = .MASK_LEN_LOAD (vectp_a.12_54, 8B, mask__24.11_52, _67, 0);

Is there a way to simplify that with the convert?

Or maybe we want to it if one or the other conversion simplifies ....

Confirmed.

[Bug tree-optimization/111114] RISC-V: Failed combine extend + vcond_mask when modify by vect_recog_over_widening_pattern

[lehua.ding at rivai dot ai]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111114

--- Comment #2 from Lehua Ding <lehua.ding at rivai dot ai> ---
(In reply to Andrew Pinski from comment #1)
> There is a match pattern which handles the case where the convert (cast)
> will fold both sides of the vec_cond now (since r14-3337-g70c50c87273d94).
> 
>   vect__5.14_56 = .MASK_LEN_LOAD (vectp_a.12_54, 8B, mask__24.11_52, _67, 0);
> 
> Is there a way to simplify that with the convert?
> 
> Or maybe we want to it if one or the other conversion simplifies ....
> 
> Confirmed.

Thanks for the comments. Can we relax this match by removing the modifier "!"?

[Bug tree-optimization/111114] RISC-V: Failed combine extend + vcond_mask when modify by vect_recog_over_widening_pattern

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111114

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Lehua Ding from comment #2)
> (In reply to Andrew Pinski from comment #1)
> > There is a match pattern which handles the case where the convert (cast)
> > will fold both sides of the vec_cond now (since r14-3337-g70c50c87273d94).
> > 
> >   vect__5.14_56 = .MASK_LEN_LOAD (vectp_a.12_54, 8B, mask__24.11_52, _67, 0);
> > 
> > Is there a way to simplify that with the convert?
> > 
> > Or maybe we want to it if one or the other conversion simplifies ....
> > 
> > Confirmed.
> 
> Thanks for the comments. Can we relax this match by removing the modifier
> "!"?

I don't think so as we want to make sure we don't replace one operation
happening with 2. The check really should be if one side will fold.

We can special case VECTOR_CST here knowing that the convert will fold the CST
always.
So something like:
```
(simplify
 (convert (vec_cond:s @0 @1 @2))
 (if (VECTOR_TYPE_P (type)
      && types_match (TREE_TYPE (@0), truth_type_for (type)))
  (if (TREE_CODE (@1) == VECTOR_CST || TREE_CODE (@2) == VECTOR_CST)
   (vec_cond @0 (convert @1) (convert @2)
   (vec_cond @0 (convert! @1) (convert! @2)))))
```
We should do a similar trick to both the unary operator and view_convert too.

[Bug tree-optimization/111114] RISC-V: Failed combine extend + vcond_mask when modify by vect_recog_over_widening_pattern

[lehua.ding at rivai dot ai]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111114

--- Comment #4 from Lehua Ding <lehua.ding at rivai dot ai> ---
(In reply to Andrew Pinski from comment #3)
> (In reply to Lehua Ding from comment #2)
> > (In reply to Andrew Pinski from comment #1)
> > > There is a match pattern which handles the case where the convert (cast)
> > > will fold both sides of the vec_cond now (since r14-3337-g70c50c87273d94).
> > > 
> > >   vect__5.14_56 = .MASK_LEN_LOAD (vectp_a.12_54, 8B, mask__24.11_52, _67, 0);
> > > 
> > > Is there a way to simplify that with the convert?
> > > 
> > > Or maybe we want to it if one or the other conversion simplifies ....
> > > 
> > > Confirmed.
> > 
> > Thanks for the comments. Can we relax this match by removing the modifier
> > "!"?
> 
> I don't think so as we want to make sure we don't replace one operation
> happening with 2. The check really should be if one side will fold.
> 
> We can special case VECTOR_CST here knowing that the convert will fold the
> CST always.
> So something like:
> ```
> (simplify
>  (convert (vec_cond:s @0 @1 @2))
>  (if (VECTOR_TYPE_P (type)
>       && types_match (TREE_TYPE (@0), truth_type_for (type)))
>   (if (TREE_CODE (@1) == VECTOR_CST || TREE_CODE (@2) == VECTOR_CST)
>    (vec_cond @0 (convert @1) (convert @2)
>    (vec_cond @0 (convert! @1) (convert! @2)))))
> ```
> We should do a similar trick to both the unary operator and view_convert too.

Just to double check, are you saying something like the bellow? But I don't
feel like the purpose is quite the same though. The match here was supposed to
remove the CONVERT operation. Whereas I was hoping to reverse the order in
order to combine them. 

```
(simplify
 (convert (vec_cond:s @0 @1 @2))
 (if (VECTOR_TYPE_P (type)
      && types_match (TREE_TYPE (@0), truth_type_for (type))
      && (TREE_CODE (@1) == VECTOR_CST || TREE_CODE (@2) == VECTOR_CST))
   (vec_cond @0 (convert @1) (convert @2))))
```

[Bug tree-optimization/111114] RISC-V: Failed combine extend + vcond_mask when modify by vect_recog_over_widening_pattern

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111114

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Lehua Ding from comment #4)
> Just to double check, are you saying something like the bellow? But I don't
> feel like the purpose is quite the same though. The match here was supposed
> to remove the CONVERT operation. Whereas I was hoping to reverse the order
> in order to combine them. 
> 
> ```
> (simplify
>  (convert (vec_cond:s @0 @1 @2))
>  (if (VECTOR_TYPE_P (type)
>       && types_match (TREE_TYPE (@0), truth_type_for (type))
>       && (TREE_CODE (@1) == VECTOR_CST || TREE_CODE (@2) == VECTOR_CST))
>    (vec_cond @0 (convert @1) (convert @2))))
> ```

Before any changes, we currently get:
  vect__5.14_56 = .MASK_LEN_LOAD (vectp_a.12_54, 8B, mask__24.11_52, _67, 0);
  vect_patt_40.15_58 = VEC_COND_EXPR <mask__24.11_52, vect__5.14_56, { 1, 1, 1,
1, 1, 1, 1, 1 }>;
  vect_patt_41.16_59 = (vector(8) short unsigned int) vect_patt_40.15_58;

I assumed you want to push that convert to be before the VEC_COND_EXPR,
correct?
If so the above match pattern should do that. The check for VECTOR_CST here is
mainly so we can the same # of convert expressions show up.

[Bug tree-optimization/111114] RISC-V: Failed combine extend + vcond_mask when modify by vect_recog_over_widening_pattern

[lehua.ding at rivai dot ai]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111114

--- Comment #6 from Lehua Ding <lehua.ding at rivai dot ai> ---
(In reply to Andrew Pinski from comment #5)
> (In reply to Lehua Ding from comment #4)
> > Just to double check, are you saying something like the bellow? But I don't
> > feel like the purpose is quite the same though. The match here was supposed
> > to remove the CONVERT operation. Whereas I was hoping to reverse the order
> > in order to combine them. 
> > 
> > ```
> > (simplify
> >  (convert (vec_cond:s @0 @1 @2))
> >  (if (VECTOR_TYPE_P (type)
> >       && types_match (TREE_TYPE (@0), truth_type_for (type))
> >       && (TREE_CODE (@1) == VECTOR_CST || TREE_CODE (@2) == VECTOR_CST))
> >    (vec_cond @0 (convert @1) (convert @2))))
> > ```
> 
> Before any changes, we currently get:
>   vect__5.14_56 = .MASK_LEN_LOAD (vectp_a.12_54, 8B, mask__24.11_52, _67, 0);
>   vect_patt_40.15_58 = VEC_COND_EXPR <mask__24.11_52, vect__5.14_56, { 1, 1,
> 1, 1, 1, 1, 1, 1 }>;
>   vect_patt_41.16_59 = (vector(8) short unsigned int) vect_patt_40.15_58;
> 
> I assumed you want to push that convert to be before the VEC_COND_EXPR,
> correct?

Yes, By this I mean that changing the order does not reduce the instructions,
so it does not feel easy to assess whether it is reasonable or not. Because
vect_recog_over_widening_pattern considers vcond_mask + convert to be more
efficient than convert + vcond_mask. What do you think?