Re: [PATCH] s390: Implement vec_set with vec_merge and, vec_duplicate.
On 8/12/22 16:48, Robin Dapp wrote:
> Hi,
>
> similar to other backends this patch implements vec_set via
> vec_merge and vec_duplicate instead of an unspec. This opens up
> more possibilites to combine instructions.
>
> Bootstrapped and regtested. No regressions.
>
> Is it OK?
>
> Regards
> Robin
>
> gcc/ChangeLog:
>
> * config/s390/s390.md: Implement vec_set with vec_merge and
> vec_duplicate.
> * config/s390/vector.md: Likewise.
> * config/s390/vx-builtins.md: Likewise.
> * config/s390/s390.cc (s390_expand_vec_init): Emit new pattern.
> (print_operand_address): New output modifier.
> (print_operand): New output modifier.
The way you handle the element selector doesn't look right to me. It appears to
be an index if it is
a CONST_INT and a bitmask otherwise. I don't think it is legal to change
operand semantics like this
depending on the operand type. This would break e.g. if LRA would decide to
load the immediate index
in a register.
Couldn't you make the shift part of the RTX instead and have the parameter
always as an index?
Bye,
Andreas
> ---
>
> diff --git a/gcc/config/s390/s390.cc b/gcc/config/s390/s390.cc
> index c86b26933d7a..ff89fb83360a 100644
> --- a/gcc/config/s390/s390.cc
> +++ b/gcc/config/s390/s390.cc
> @@ -7073,11 +7073,10 @@ s390_expand_vec_init (rtx target, rtx vals)
>if (!general_operand (elem, GET_MODE (elem)))
> elem = force_reg (inner_mode, elem);
>
> - emit_insn (gen_rtx_SET (target,
> - gen_rtx_UNSPEC (mode,
> - gen_rtvec (3, elem,
> - GEN_INT (i), target),
> - UNSPEC_VEC_SET)));
> + emit_insn
> + (gen_rtx_SET
> + (target, gen_rtx_VEC_MERGE
> + (mode, gen_rtx_VEC_DUPLICATE (mode, elem), target, GEN_INT (1 <<
> i;
> }
> }
>
> @@ -8057,6 +8056,8 @@ print_operand_address (FILE *file, rtx addr)
> 'S': print S-type memory reference (base+displacement).
> 'Y': print address style operand without index (e.g. shift count or
> setmem
>operand).
> +'P': print address-style operand without index but with the offset as
> + if it were specified by a 'p' format flag.
>
> 'b': print integer X as if it's an unsigned byte.
> 'c': print integer X as if it's an signed byte.
> @@ -8068,6 +8069,7 @@ print_operand_address (FILE *file, rtx addr)
> 'k': print the first nonzero SImode part of X.
> 'm': print the first SImode part unequal to -1 of X.
> 'o': print integer X as if it's an unsigned 32bit word.
> +'p': print N such that 2^N == X (X must be a power of 2 and const int).
> 's': "start" of contiguous bitmask X in either DImode or vector
> inner mode.
> 't': CONST_INT: "start" of contiguous bitmask X in SImode.
>CONST_VECTOR: Generate a bitmask for vgbm instruction.
> @@ -8237,6 +8239,16 @@ print_operand (FILE *file, rtx x, int code)
>print_shift_count_operand (file, x);
>return;
>
> +case 'P':
> + if (CONST_INT_P (x))
> + {
> + ival = exact_log2 (INTVAL (x));
> + fprintf (file, HOST_WIDE_INT_PRINT_DEC, ival);
> + }
> + else
> + print_shift_count_operand (file, x);
> + return;
> +
> case 'K':
>/* Append @PLT to both local and non-local symbols in order to
> support
>Linux Kernel livepatching: patches contain individual functions and
> @@ -8321,6 +8333,9 @@ print_operand (FILE *file, rtx x, int code)
> case 'o':
> ival &= 0x;
> break;
> + case 'p':
> + ival = exact_log2 (INTVAL (x));
> + break;
> case 'e': case 'f':
> case 's': case 't':
> {
> diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
> index f37d8fd33a15..a82db4c624fa 100644
> --- a/gcc/config/s390/s390.md
> +++ b/gcc/config/s390/s390.md
> @@ -183,7 +183,6 @@ (define_c_enum "unspec" [
> UNSPEC_VEC_GFMSUM_128
> UNSPEC_VEC_GFMSUM_ACCUM
> UNSPEC_VEC_GFMSUM_ACCUM_128
> - UNSPEC_VEC_SET
>
> UNSPEC_VEC_VSUMG
> UNSPEC_VEC_VSUMQ
> diff --git a/gcc/config/s390/vector.md b/gcc/config/s390/vector.md
> index c50451a8326c..bde3a39db3d4 100644
> --- a/gcc/config/s390/vector.md
> +++ b/gcc/config/s390/vector.md
> @@ -467,12 +467,17 @@ (define_insn "mov"
> ; vec_set is supposed to *modify* an existing vector so operand 0 is
> ; duplicated as input operand.
> (define_expand "vec_set"
> - [(set (match_operand:V0 "register_operand" "")
> - (unspec:V [(match_operand: 1 "general_operand" "")
> -(match_operand:SI2 "nonmemory_operand" "")
> -(match_dup 0)]
> -UNSPEC_VEC_SET))]
> - "TARGET_VX")
> + [(set (match_operand:V 0 "register_operand" "")
> + (vec_merge:V
> + (vec_duplicate:V
> + (match_operand: 1 "general_operand" ""))
[PATCH] s390: Implement vec_set with vec_merge and, vec_duplicate.
Hi,
similar to other backends this patch implements vec_set via
vec_merge and vec_duplicate instead of an unspec. This opens up
more possibilites to combine instructions.
Bootstrapped and regtested. No regressions.
Is it OK?
Regards
Robin
gcc/ChangeLog:
* config/s390/s390.md: Implement vec_set with vec_merge and
vec_duplicate.
* config/s390/vector.md: Likewise.
* config/s390/vx-builtins.md: Likewise.
* config/s390/s390.cc (s390_expand_vec_init): Emit new pattern.
(print_operand_address): New output modifier.
(print_operand): New output modifier.
---
diff --git a/gcc/config/s390/s390.cc b/gcc/config/s390/s390.cc
index c86b26933d7a..ff89fb83360a 100644
--- a/gcc/config/s390/s390.cc
+++ b/gcc/config/s390/s390.cc
@@ -7073,11 +7073,10 @@ s390_expand_vec_init (rtx target, rtx vals)
if (!general_operand (elem, GET_MODE (elem)))
elem = force_reg (inner_mode, elem);
- emit_insn (gen_rtx_SET (target,
- gen_rtx_UNSPEC (mode,
- gen_rtvec (3, elem,
-GEN_INT (i), target),
- UNSPEC_VEC_SET)));
+ emit_insn
+ (gen_rtx_SET
+(target, gen_rtx_VEC_MERGE
+ (mode, gen_rtx_VEC_DUPLICATE (mode, elem), target, GEN_INT (1 <<
i;
}
}
@@ -8057,6 +8056,8 @@ print_operand_address (FILE *file, rtx addr)
'S': print S-type memory reference (base+displacement).
'Y': print address style operand without index (e.g. shift count or
setmem
operand).
+'P': print address-style operand without index but with the offset as
+if it were specified by a 'p' format flag.
'b': print integer X as if it's an unsigned byte.
'c': print integer X as if it's an signed byte.
@@ -8068,6 +8069,7 @@ print_operand_address (FILE *file, rtx addr)
'k': print the first nonzero SImode part of X.
'm': print the first SImode part unequal to -1 of X.
'o': print integer X as if it's an unsigned 32bit word.
+'p': print N such that 2^N == X (X must be a power of 2 and const int).
's': "start" of contiguous bitmask X in either DImode or vector
inner mode.
't': CONST_INT: "start" of contiguous bitmask X in SImode.
CONST_VECTOR: Generate a bitmask for vgbm instruction.
@@ -8237,6 +8239,16 @@ print_operand (FILE *file, rtx x, int code)
print_shift_count_operand (file, x);
return;
+case 'P':
+ if (CONST_INT_P (x))
+ {
+ ival = exact_log2 (INTVAL (x));
+ fprintf (file, HOST_WIDE_INT_PRINT_DEC, ival);
+ }
+ else
+ print_shift_count_operand (file, x);
+ return;
+
case 'K':
/* Append @PLT to both local and non-local symbols in order to
support
Linux Kernel livepatching: patches contain individual functions and
@@ -8321,6 +8333,9 @@ print_operand (FILE *file, rtx x, int code)
case 'o':
ival &= 0x;
break;
+ case 'p':
+ ival = exact_log2 (INTVAL (x));
+ break;
case 'e': case 'f':
case 's': case 't':
{
diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
index f37d8fd33a15..a82db4c624fa 100644
--- a/gcc/config/s390/s390.md
+++ b/gcc/config/s390/s390.md
@@ -183,7 +183,6 @@ (define_c_enum "unspec" [
UNSPEC_VEC_GFMSUM_128
UNSPEC_VEC_GFMSUM_ACCUM
UNSPEC_VEC_GFMSUM_ACCUM_128
- UNSPEC_VEC_SET
UNSPEC_VEC_VSUMG
UNSPEC_VEC_VSUMQ
diff --git a/gcc/config/s390/vector.md b/gcc/config/s390/vector.md
index c50451a8326c..bde3a39db3d4 100644
--- a/gcc/config/s390/vector.md
+++ b/gcc/config/s390/vector.md
@@ -467,12 +467,17 @@ (define_insn "mov"
; vec_set is supposed to *modify* an existing vector so operand 0 is
; duplicated as input operand.
(define_expand "vec_set"
- [(set (match_operand:V0 "register_operand" "")
- (unspec:V [(match_operand: 1 "general_operand" "")
- (match_operand:SI2 "nonmemory_operand" "")
- (match_dup 0)]
- UNSPEC_VEC_SET))]
- "TARGET_VX")
+ [(set (match_operand:V0 "register_operand" "")
+ (vec_merge:V
+ (vec_duplicate:V
+ (match_operand: 1 "general_operand" ""))
+ (match_dup 0)
+ (match_operand:SI 2 "nonmemory_operand")))]
+ ""
+{
+ if (CONST_INT_P (operands[2]))
+operands[2] = GEN_INT (1 << INTVAL (operands[2]));
+})
; FIXME: Support also vector mode operands for 1
; FIXME: A target memory operand seems to be useful otherwise we end
@@ -480,28 +485,31 @@ (define_expand "vec_set"
; that itself?
; vlvgb, vlvgh, vlvgf, vlvgg, vleb, vleh, vlef, vleg, vleib, vleih,
vleif, vleig
(define_insn "*vec_set"
- [(set (match_operand:V0 "register_operand" "=v,v,v")
- (unspec:V [(match_operand: 1 "general_operand""d,R,K")
-
