Re: [PATCH] rs6000: Fix wrong code generation for vec_sel [PR94613]

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Fri, Apr 30, 2021 at 01:32:58AM -0500, Xionghu Luo wrote:
> The vsel instruction is a bit-wise select instruction.  Using an
> IF_THEN_ELSE to express it in RTL is wrong and leads to wrong code
> being generated in the combine pass.  Per element selection is a
> subset of per bit-wise selection,with the patch the pattern is
> written using bit operations.  But there are 8 different patterns
> to define "op0 := (op1 & ~op3) | (op2 & op3)":
> 
> (~op3&op1) | (op3&op2),
> (~op3&op1) | (op2&op3),
> (op3&op2) | (~op3&op1),
> (op2&op3) | (~op3&op1),
> (op1&~op3) | (op3&op2),
> (op1&~op3) | (op2&op3),
> (op3&op2) | (op1&~op3),
> (op2&op3) | (op1&~op3),
> 
> Combine pass will swap (op1&~op3) to (~op3&op1) due to commutative
> canonical, which could reduce it to the FIRST 4 patterns, but it won't
> swap (op2&op3) | (~op3&op1) to (~op3&op1) | (op2&op3), so this patch
> handles it with two patterns with different NOT op3 position and check
> equality inside it.

Yup, that latter case does not have canonicalisation rules.  Btw, not
only combine does this canonicalisation: everything should,
non-canonical RTL is invalid RTL (in the instruction stream, you can do
everything in temporary code of course, as long as the RTL isn't
malformed).

> -(define_insn "*altivec_vsel<mode>"
> +(define_insn "altivec_vsel<mode>"
>    [(set (match_operand:VM 0 "altivec_register_operand" "=v")
> -	(if_then_else:VM
> -	 (ne:CC (match_operand:VM 1 "altivec_register_operand" "v")
> -		(match_operand:VM 4 "zero_constant" ""))
> -	 (match_operand:VM 2 "altivec_register_operand" "v")
> -	 (match_operand:VM 3 "altivec_register_operand" "v")))]
> -  "VECTOR_MEM_ALTIVEC_P (<MODE>mode)"
> -  "vsel %0,%3,%2,%1"
> +	(ior:VM
> +	 (and:VM
> +	  (not:VM (match_operand:VM 3 "altivec_register_operand" "v"))
> +	  (match_operand:VM 1 "altivec_register_operand" "v"))
> +	 (and:VM
> +	  (match_operand:VM 2 "altivec_register_operand" "v")
> +	  (match_operand:VM 4 "altivec_register_operand" "v"))))]
> +  "VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode)
> +  && (rtx_equal_p (operands[2], operands[3])
> +  || rtx_equal_p (operands[4], operands[3]))"
> +  {
> +    if (rtx_equal_p (operands[2], operands[3]))
> +      return "vsel %0,%1,%4,%3";
> +    else
> +      return "vsel %0,%1,%2,%3";
> +  }
>    [(set_attr "type" "vecmove")])

That rtx_equal_p stuff is nice and tricky, but it is a bit too tricky I
think.  So please write this as two patterns (and keep the expand if
that helps).

> +(define_insn "altivec_vsel<mode>2"

(same here of course).

>  ;; Fused multiply add.
> diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
> index f5676255387..d65bdc01055 100644
> --- a/gcc/config/rs6000/rs6000-call.c
> +++ b/gcc/config/rs6000/rs6000-call.c
> @@ -3362,11 +3362,11 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = {
>      RS6000_BTI_V2DI, RS6000_BTI_V2DI, RS6000_BTI_V2DI, RS6000_BTI_unsigned_V2DI },
>    { ALTIVEC_BUILTIN_VEC_SEL, ALTIVEC_BUILTIN_VSEL_2DI,
>      RS6000_BTI_V2DI, RS6000_BTI_V2DI, RS6000_BTI_V2DI, RS6000_BTI_V2DI },
> -  { ALTIVEC_BUILTIN_VEC_SEL, ALTIVEC_BUILTIN_VSEL_2DI,
> +  { ALTIVEC_BUILTIN_VEC_SEL, ALTIVEC_BUILTIN_VSEL_2DI_UNS,

Are the _uns things still used for anything?  But, let's not change
this until Bill's stuff is in :-)

Why do you want to change this here, btw?  I don't understand.

> +  if (target == 0
> +      || GET_MODE (target) != tmode
> +      || ! (*insn_data[icode].operand[0].predicate) (target, tmode))

No space after ! and other unary operators (except for casts and other
operators you write with alphanumerics, like "sizeof").  I know you
copied this code, but :-)

> @@ -15608,8 +15606,6 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx op_true, rtx op_false,
>      case GEU:
>      case LTU:
>      case LEU:
> -      /* Mark unsigned tests with CCUNSmode.  */
> -      cc_mode = CCUNSmode;
>  
>        /* Invert condition to avoid compound test if necessary.  */
>        if (rcode == GEU || rcode == LEU)

So this is related to the _uns thing.  Could you split off that change?
Probably as an earlier patch (but either works for me).

> @@ -15629,6 +15625,9 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx op_true, rtx op_false,
>    if (!mask)
>      return 0;
>  
> +  if (mask_mode != dest_mode)
> +      mask = simplify_gen_subreg (dest_mode, mask, mask_mode, 0);

Indent just two characters please: line continuations (usually) align,
but indents do not.


Can you fold vsel and xxsel together completely?  They have exactly the
same semantics!  This does not have to be in this patch of course.

Thanks,


Segher