* [PATCH] Improve V?TImode shifts (PR target/82370)
@ 2017-10-04 19:35 Jakub Jelinek
2017-10-04 21:23 ` H.J. Lu
2017-10-20 19:48 ` Kirill Yukhin
0 siblings, 2 replies; 7+ messages in thread
From: Jakub Jelinek @ 2017-10-04 19:35 UTC (permalink / raw)
To: Uros Bizjak, Kirill Yukhin, H.J. Lu; +Cc: gcc-patches
Hi!
The following patch tweaks the TImode vector shifts similarly
to the earlier vector shift patch, so that for shifts by immediate
we can accept a memory input. Additionally, it removes the vec_shl_*
expander, because the middle-end has dropped that a few years ago,
and merges the left and right shift patterns using code iterators.
Appart from the code/names that can be handled by mode attributes,
the only difference was that one of the insns had
(set_attr "atom_unit" "sishuf")
and the other didn't. I hope that is just an error, I'd really expect
both vpslldq and vpsrldq to use the same atom unit, isn't that the case?
CCing H.J. for that. If it is intentional difference, I can of course
adjust the patch and undo the merging of the 4 define_insns into 2.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2017-10-04 Jakub Jelinek <jakub@redhat.com>
PR target/82370
* config/i386/sse.md (VIMAX_AVX2): Remove V4TImode.
(VIMAX_AVX2_AVX512BW, VIMAX_AVX512VL): New mode iterators.
(vec_shl_<mode>): Remove unused expander.
(avx512bw_<shift_insn><mode>3): New define_insn.
(<sse2_avx2>_ashl<mode>3, <sse2_avx2>_lshr<mode>3): Replaced by ...
(<sse2_avx2>_<shift_insn><mode>3): ... this. New define_insn.
* gcc.target/i386/pr82370.c: New test.
--- gcc/config/i386/sse.md.jj 2017-10-04 12:18:19.000000000 +0200
+++ gcc/config/i386/sse.md 2017-10-04 15:34:00.541860351 +0200
@@ -371,10 +371,17 @@ (define_mode_iterator V16FI
[V16SF V16SI])
;; ??? We should probably use TImode instead.
-(define_mode_iterator VIMAX_AVX2
+(define_mode_iterator VIMAX_AVX2_AVX512BW
[(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") V1TI])
-;; ??? This should probably be dropped in favor of VIMAX_AVX2.
+;; Suppose TARGET_AVX512BW as baseline
+(define_mode_iterator VIMAX_AVX512VL
+ [V4TI (V2TI "TARGET_AVX512VL") (V1TI "TARGET_AVX512VL")])
+
+(define_mode_iterator VIMAX_AVX2
+ [(V2TI "TARGET_AVX2") V1TI])
+
+;; ??? This should probably be dropped in favor of VIMAX_AVX2_AVX512BW.
(define_mode_iterator SSESCALARMODE
[(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") TI])
@@ -10792,9 +10799,9 @@ (define_insn "<shift_insn><mode>3<mask_n
(set_attr "mode" "<sseinsnmode>")])
-(define_expand "vec_shl_<mode>"
+(define_expand "vec_shr_<mode>"
[(set (match_dup 3)
- (ashift:V1TI
+ (lshiftrt:V1TI
(match_operand:VI_128 1 "register_operand")
(match_operand:SI 2 "const_0_to_255_mul_8_operand")))
(set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
@@ -10805,48 +10812,24 @@ (define_expand "vec_shl_<mode>"
operands[4] = gen_lowpart (<MODE>mode, operands[3]);
})
-(define_insn "<sse2_avx2>_ashl<mode>3"
- [(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
- (ashift:VIMAX_AVX2
- (match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
- (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
- "TARGET_SSE2"
+(define_insn "avx512bw_<shift_insn><mode>3"
+ [(set (match_operand:VIMAX_AVX512VL 0 "register_operand" "=v")
+ (any_lshift:VIMAX_AVX512VL
+ (match_operand:VIMAX_AVX512VL 1 "nonimmediate_operand" "vm")
+ (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n")))]
+ "TARGET_AVX512BW"
{
operands[2] = GEN_INT (INTVAL (operands[2]) / 8);
-
- switch (which_alternative)
- {
- case 0:
- return "pslldq\t{%2, %0|%0, %2}";
- case 1:
- return "vpslldq\t{%2, %1, %0|%0, %1, %2}";
- default:
- gcc_unreachable ();
- }
+ return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
}
- [(set_attr "isa" "noavx,avx")
- (set_attr "type" "sseishft")
+ [(set_attr "type" "sseishft")
(set_attr "length_immediate" "1")
- (set_attr "prefix_data16" "1,*")
- (set_attr "prefix" "orig,vex")
+ (set_attr "prefix" "maybe_evex")
(set_attr "mode" "<sseinsnmode>")])
-(define_expand "vec_shr_<mode>"
- [(set (match_dup 3)
- (lshiftrt:V1TI
- (match_operand:VI_128 1 "register_operand")
- (match_operand:SI 2 "const_0_to_255_mul_8_operand")))
- (set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
- "TARGET_SSE2"
-{
- operands[1] = gen_lowpart (V1TImode, operands[1]);
- operands[3] = gen_reg_rtx (V1TImode);
- operands[4] = gen_lowpart (<MODE>mode, operands[3]);
-})
-
-(define_insn "<sse2_avx2>_lshr<mode>3"
+(define_insn "<sse2_avx2>_<shift_insn><mode>3"
[(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
- (lshiftrt:VIMAX_AVX2
+ (any_lshift:VIMAX_AVX2
(match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
(match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
"TARGET_SSE2"
@@ -10856,9 +10839,9 @@ (define_insn "<sse2_avx2>_lshr<mode>3"
switch (which_alternative)
{
case 0:
- return "psrldq\t{%2, %0|%0, %2}";
+ return "p<vshift>dq\t{%2, %0|%0, %2}";
case 1:
- return "vpsrldq\t{%2, %1, %0|%0, %1, %2}";
+ return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
default:
gcc_unreachable ();
}
--- gcc/testsuite/gcc.target/i386/pr82370.c.jj 2017-10-04 16:01:16.350247297 +0200
+++ gcc/testsuite/gcc.target/i386/pr82370.c 2017-10-04 16:03:06.704922288 +0200
@@ -0,0 +1,18 @@
+/* PR target/82370 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -mavx512vl -mavx512bw -masm=att" } */
+/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
+
+#include <x86intrin.h>
+
+__m512i f1 (__m512i *x) { return _mm512_bslli_epi128 (*x, 5); }
+__m512i f2 (__m512i *x) { return _mm512_bsrli_epi128 (*x, 5); }
+__m256i f3 (__m256i *x) { return _mm256_bslli_epi128 (*x, 5); }
+__m256i f4 (__m256i *x) { return _mm256_bsrli_epi128 (*x, 5); }
+__m128i f5 (__m128i *x) { return _mm_bslli_si128 (*x, 5); }
+__m128i f6 (__m128i *x) { return _mm_bsrli_si128 (*x, 5); }
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve V?TImode shifts (PR target/82370)
2017-10-04 19:35 [PATCH] Improve V?TImode shifts (PR target/82370) Jakub Jelinek
@ 2017-10-04 21:23 ` H.J. Lu
2017-10-20 19:48 ` Kirill Yukhin
1 sibling, 0 replies; 7+ messages in thread
From: H.J. Lu @ 2017-10-04 21:23 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Uros Bizjak, Kirill Yukhin, gcc-patches
On 10/4/17, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The following patch tweaks the TImode vector shifts similarly
> to the earlier vector shift patch, so that for shifts by immediate
> we can accept a memory input. Additionally, it removes the vec_shl_*
I prefer 2 patches, a separate patch to drop vec_shl_* first. which can
go in now.
Thanks.
> expander, because the middle-end has dropped that a few years ago,
> and merges the left and right shift patterns using code iterators.
> Appart from the code/names that can be handled by mode attributes,
> the only difference was that one of the insns had
> (set_attr "atom_unit" "sishuf")
> and the other didn't. I hope that is just an error, I'd really expect
> both vpslldq and vpsrldq to use the same atom unit, isn't that the case?
> CCing H.J. for that. If it is intentional difference, I can of course
> adjust the patch and undo the merging of the 4 define_insns into 2.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2017-10-04 Jakub Jelinek <jakub@redhat.com>
>
> PR target/82370
> * config/i386/sse.md (VIMAX_AVX2): Remove V4TImode.
> (VIMAX_AVX2_AVX512BW, VIMAX_AVX512VL): New mode iterators.
> (vec_shl_<mode>): Remove unused expander.
> (avx512bw_<shift_insn><mode>3): New define_insn.
> (<sse2_avx2>_ashl<mode>3, <sse2_avx2>_lshr<mode>3): Replaced by ...
> (<sse2_avx2>_<shift_insn><mode>3): ... this. New define_insn.
>
> * gcc.target/i386/pr82370.c: New test.
>
> --- gcc/config/i386/sse.md.jj 2017-10-04 12:18:19.000000000 +0200
> +++ gcc/config/i386/sse.md 2017-10-04 15:34:00.541860351 +0200
> @@ -371,10 +371,17 @@ (define_mode_iterator V16FI
> [V16SF V16SI])
>
> ;; ??? We should probably use TImode instead.
> -(define_mode_iterator VIMAX_AVX2
> +(define_mode_iterator VIMAX_AVX2_AVX512BW
> [(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") V1TI])
>
> -;; ??? This should probably be dropped in favor of VIMAX_AVX2.
> +;; Suppose TARGET_AVX512BW as baseline
> +(define_mode_iterator VIMAX_AVX512VL
> + [V4TI (V2TI "TARGET_AVX512VL") (V1TI "TARGET_AVX512VL")])
> +
> +(define_mode_iterator VIMAX_AVX2
> + [(V2TI "TARGET_AVX2") V1TI])
> +
> +;; ??? This should probably be dropped in favor of VIMAX_AVX2_AVX512BW.
> (define_mode_iterator SSESCALARMODE
> [(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") TI])
>
> @@ -10792,9 +10799,9 @@ (define_insn "<shift_insn><mode>3<mask_n
> (set_attr "mode" "<sseinsnmode>")])
>
>
> -(define_expand "vec_shl_<mode>"
> +(define_expand "vec_shr_<mode>"
> [(set (match_dup 3)
> - (ashift:V1TI
> + (lshiftrt:V1TI
> (match_operand:VI_128 1 "register_operand")
> (match_operand:SI 2 "const_0_to_255_mul_8_operand")))
> (set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
> @@ -10805,48 +10812,24 @@ (define_expand "vec_shl_<mode>"
> operands[4] = gen_lowpart (<MODE>mode, operands[3]);
> })
>
> -(define_insn "<sse2_avx2>_ashl<mode>3"
> - [(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
> - (ashift:VIMAX_AVX2
> - (match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
> - (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
> - "TARGET_SSE2"
> +(define_insn "avx512bw_<shift_insn><mode>3"
> + [(set (match_operand:VIMAX_AVX512VL 0 "register_operand" "=v")
> + (any_lshift:VIMAX_AVX512VL
> + (match_operand:VIMAX_AVX512VL 1 "nonimmediate_operand" "vm")
> + (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n")))]
> + "TARGET_AVX512BW"
> {
> operands[2] = GEN_INT (INTVAL (operands[2]) / 8);
> -
> - switch (which_alternative)
> - {
> - case 0:
> - return "pslldq\t{%2, %0|%0, %2}";
> - case 1:
> - return "vpslldq\t{%2, %1, %0|%0, %1, %2}";
> - default:
> - gcc_unreachable ();
> - }
> + return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
> }
> - [(set_attr "isa" "noavx,avx")
> - (set_attr "type" "sseishft")
> + [(set_attr "type" "sseishft")
> (set_attr "length_immediate" "1")
> - (set_attr "prefix_data16" "1,*")
> - (set_attr "prefix" "orig,vex")
> + (set_attr "prefix" "maybe_evex")
> (set_attr "mode" "<sseinsnmode>")])
>
> -(define_expand "vec_shr_<mode>"
> - [(set (match_dup 3)
> - (lshiftrt:V1TI
> - (match_operand:VI_128 1 "register_operand")
> - (match_operand:SI 2 "const_0_to_255_mul_8_operand")))
> - (set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
> - "TARGET_SSE2"
> -{
> - operands[1] = gen_lowpart (V1TImode, operands[1]);
> - operands[3] = gen_reg_rtx (V1TImode);
> - operands[4] = gen_lowpart (<MODE>mode, operands[3]);
> -})
> -
> -(define_insn "<sse2_avx2>_lshr<mode>3"
> +(define_insn "<sse2_avx2>_<shift_insn><mode>3"
> [(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
> - (lshiftrt:VIMAX_AVX2
> + (any_lshift:VIMAX_AVX2
> (match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
> (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
> "TARGET_SSE2"
> @@ -10856,9 +10839,9 @@ (define_insn "<sse2_avx2>_lshr<mode>3"
> switch (which_alternative)
> {
> case 0:
> - return "psrldq\t{%2, %0|%0, %2}";
> + return "p<vshift>dq\t{%2, %0|%0, %2}";
> case 1:
> - return "vpsrldq\t{%2, %1, %0|%0, %1, %2}";
> + return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
> default:
> gcc_unreachable ();
> }
> --- gcc/testsuite/gcc.target/i386/pr82370.c.jj 2017-10-04 16:01:16.350247297
> +0200
> +++ gcc/testsuite/gcc.target/i386/pr82370.c 2017-10-04 16:03:06.704922288
> +0200
> @@ -0,0 +1,18 @@
> +/* PR target/82370 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mavx512vl -mavx512bw -masm=att" } */
> +/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5,
> \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5,
> \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5,
> \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5,
> \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5,
> \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5,
> \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
> +
> +#include <x86intrin.h>
> +
> +__m512i f1 (__m512i *x) { return _mm512_bslli_epi128 (*x, 5); }
> +__m512i f2 (__m512i *x) { return _mm512_bsrli_epi128 (*x, 5); }
> +__m256i f3 (__m256i *x) { return _mm256_bslli_epi128 (*x, 5); }
> +__m256i f4 (__m256i *x) { return _mm256_bsrli_epi128 (*x, 5); }
> +__m128i f5 (__m128i *x) { return _mm_bslli_si128 (*x, 5); }
> +__m128i f6 (__m128i *x) { return _mm_bsrli_si128 (*x, 5); }
>
> Jakub
>
--
H.J.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve V?TImode shifts (PR target/82370)
2017-10-04 19:35 [PATCH] Improve V?TImode shifts (PR target/82370) Jakub Jelinek
2017-10-04 21:23 ` H.J. Lu
@ 2017-10-20 19:48 ` Kirill Yukhin
2017-10-24 12:47 ` H.J. Lu
1 sibling, 1 reply; 7+ messages in thread
From: Kirill Yukhin @ 2017-10-20 19:48 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Uros Bizjak, H.J. Lu, gcc-patches
Hello Jakub, Uroš, Jakub
On 04 Oct 21:35, Jakub Jelinek wrote:
> Hi!
>
> The following patch tweaks the TImode vector shifts similarly
> to the earlier vector shift patch, so that for shifts by immediate
> we can accept a memory input. Additionally, it removes the vec_shl_*
> expander, because the middle-end has dropped that a few years ago,
> and merges the left and right shift patterns using code iterators.
> Appart from the code/names that can be handled by mode attributes,
> the only difference was that one of the insns had
> (set_attr "atom_unit" "sishuf")
> and the other didn't. I hope that is just an error, I'd really expect
> both vpslldq and vpsrldq to use the same atom unit, isn't that the case?
This was introduced back in 2010. So, I have no info.
What I can see from config/atom.md:
;; if palignr or psrldq
(define_insn_reservation "atom_sseishft_2" 1
(and (eq_attr "cpu" "atom")
(and (eq_attr "type" "sseishft")
(and (eq_attr "atom_unit" "sishuf")
(match_operand 2 "immediate_operand"))))
"atom-simple-0")
This leads back to initial commit of atom.md.
So, discrimination of psrldq and pslldq looks intentional.
On the over hand, I see in Software Optimization Guide, Table 14-2 that
PSRLDQ and PSLLDQ occupy same line which directs both insns to port-0 (p 14-18).
So, looking from that point, definition for PSLLDQ which allow either of port-0
and port-1 looks wrong (atom-simple-either reservation).
In absence of other information, I'd play on safe side and leave things as they
occur right now.
Maybe Uroš or HJ could shed a light?
--
Thanks, K
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve V?TImode shifts (PR target/82370)
2017-10-20 19:48 ` Kirill Yukhin
@ 2017-10-24 12:47 ` H.J. Lu
2017-10-24 14:54 ` Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: H.J. Lu @ 2017-10-24 12:47 UTC (permalink / raw)
To: Kirill Yukhin; +Cc: Jakub Jelinek, Uros Bizjak, GCC Patches
On Fri, Oct 20, 2017 at 12:33 PM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
> Hello Jakub, Uroš, Jakub
> On 04 Oct 21:35, Jakub Jelinek wrote:
>> Hi!
>>
>> The following patch tweaks the TImode vector shifts similarly
>> to the earlier vector shift patch, so that for shifts by immediate
>> we can accept a memory input. Additionally, it removes the vec_shl_*
>> expander, because the middle-end has dropped that a few years ago,
>> and merges the left and right shift patterns using code iterators.
>> Appart from the code/names that can be handled by mode attributes,
>> the only difference was that one of the insns had
>> (set_attr "atom_unit" "sishuf")
>> and the other didn't. I hope that is just an error, I'd really expect
>> both vpslldq and vpsrldq to use the same atom unit, isn't that the case?
> This was introduced back in 2010. So, I have no info.
> What I can see from config/atom.md:
> ;; if palignr or psrldq
> (define_insn_reservation "atom_sseishft_2" 1
> (and (eq_attr "cpu" "atom")
> (and (eq_attr "type" "sseishft")
> (and (eq_attr "atom_unit" "sishuf")
> (match_operand 2 "immediate_operand"))))
> "atom-simple-0")
>
> This leads back to initial commit of atom.md.
> So, discrimination of psrldq and pslldq looks intentional.
>
> On the over hand, I see in Software Optimization Guide, Table 14-2 that
> PSRLDQ and PSLLDQ occupy same line which directs both insns to port-0 (p 14-18).
> So, looking from that point, definition for PSLLDQ which allow either of port-0
> and port-1 looks wrong (atom-simple-either reservation).
>
> In absence of other information, I'd play on safe side and leave things as they
> occur right now.
>
I prefer to leave atom.md ASIS. As for (set_attr "atom_unit"
"sishuf"), it was added
for
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44615
You can drop (set_attr "atom_unit" "sishuf") if gcc.target/i386/sse2-vec-2a.c
still compiles.
--
H.J.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve V?TImode shifts (PR target/82370)
2017-10-24 12:47 ` H.J. Lu
@ 2017-10-24 14:54 ` Jakub Jelinek
2017-10-24 17:35 ` Uros Bizjak
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2017-10-24 14:54 UTC (permalink / raw)
To: H.J. Lu, Kirill Yukhin; +Cc: Uros Bizjak, GCC Patches
On Tue, Oct 24, 2017 at 05:44:44AM -0700, H.J. Lu wrote:
> > What I can see from config/atom.md:
> > ;; if palignr or psrldq
> > (define_insn_reservation "atom_sseishft_2" 1
> > (and (eq_attr "cpu" "atom")
> > (and (eq_attr "type" "sseishft")
> > (and (eq_attr "atom_unit" "sishuf")
> > (match_operand 2 "immediate_operand"))))
> > "atom-simple-0")
> >
> > This leads back to initial commit of atom.md.
> > So, discrimination of psrldq and pslldq looks intentional.
> >
> > On the over hand, I see in Software Optimization Guide, Table 14-2 that
> > PSRLDQ and PSLLDQ occupy same line which directs both insns to port-0 (p 14-18).
> > So, looking from that point, definition for PSLLDQ which allow either of port-0
> > and port-1 looks wrong (atom-simple-either reservation).
> >
> > In absence of other information, I'd play on safe side and leave things as they
> > occur right now.
> >
>
> I prefer to leave atom.md ASIS. As for (set_attr "atom_unit"
> "sishuf"), it was added
> for
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44615
>
> You can drop (set_attr "atom_unit" "sishuf") if gcc.target/i386/sse2-vec-2a.c
> still compiles.
No, it was added earlier than that, that PR was about insns with psrldq with
implicit immediate (which don't have a CONST_INT operands[2]). This insn
does have it, the testcase passes regardless of whether sishuf or other is
used, it is purely a tuning thing.
In any case, here is an updated patch that just preserves the status quo
(psrldq having the sishuf unit, pslldq not) using a simple code attribute.
2017-10-24 Jakub Jelinek <jakub@redhat.com>
PR target/82370
* config/i386/sse.md (VIMAX_AVX2): Remove V4TImode.
(VIMAX_AVX2_AVX512BW, VIMAX_AVX512VL): New mode iterators.
(vec_shl_<mode>): Remove unused expander.
(avx512bw_<shift_insn><mode>3): New define_insn.
(atom_shift_unit): New code iterator.
(<sse2_avx2>_ashl<mode>3, <sse2_avx2>_lshr<mode>3): Replaced by ...
(<sse2_avx2>_<shift_insn><mode>3): ... this. New define_insn.
* gcc.target/i386/pr82370.c: New test.
--- gcc/config/i386/sse.md.jj 2017-10-20 16:30:35.286208652 +0200
+++ gcc/config/i386/sse.md 2017-10-24 16:29:54.848934888 +0200
@@ -371,10 +371,17 @@ (define_mode_iterator V16FI
[V16SF V16SI])
;; ??? We should probably use TImode instead.
-(define_mode_iterator VIMAX_AVX2
+(define_mode_iterator VIMAX_AVX2_AVX512BW
[(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") V1TI])
-;; ??? This should probably be dropped in favor of VIMAX_AVX2.
+;; Suppose TARGET_AVX512BW as baseline
+(define_mode_iterator VIMAX_AVX512VL
+ [V4TI (V2TI "TARGET_AVX512VL") (V1TI "TARGET_AVX512VL")])
+
+(define_mode_iterator VIMAX_AVX2
+ [(V2TI "TARGET_AVX2") V1TI])
+
+;; ??? This should probably be dropped in favor of VIMAX_AVX2_AVX512BW.
(define_mode_iterator SSESCALARMODE
[(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") TI])
@@ -10778,9 +10785,9 @@ (define_insn "<shift_insn><mode>3<mask_n
(set_attr "mode" "<sseinsnmode>")])
-(define_expand "vec_shl_<mode>"
+(define_expand "vec_shr_<mode>"
[(set (match_dup 3)
- (ashift:V1TI
+ (lshiftrt:V1TI
(match_operand:VI_128 1 "register_operand")
(match_operand:SI 2 "const_0_to_255_mul_8_operand")))
(set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
@@ -10791,48 +10798,26 @@ (define_expand "vec_shl_<mode>"
operands[4] = gen_lowpart (<MODE>mode, operands[3]);
})
-(define_insn "<sse2_avx2>_ashl<mode>3"
- [(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
- (ashift:VIMAX_AVX2
- (match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
- (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
- "TARGET_SSE2"
+(define_insn "avx512bw_<shift_insn><mode>3"
+ [(set (match_operand:VIMAX_AVX512VL 0 "register_operand" "=v")
+ (any_lshift:VIMAX_AVX512VL
+ (match_operand:VIMAX_AVX512VL 1 "nonimmediate_operand" "vm")
+ (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n")))]
+ "TARGET_AVX512BW"
{
operands[2] = GEN_INT (INTVAL (operands[2]) / 8);
-
- switch (which_alternative)
- {
- case 0:
- return "pslldq\t{%2, %0|%0, %2}";
- case 1:
- return "vpslldq\t{%2, %1, %0|%0, %1, %2}";
- default:
- gcc_unreachable ();
- }
+ return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
}
- [(set_attr "isa" "noavx,avx")
- (set_attr "type" "sseishft")
+ [(set_attr "type" "sseishft")
(set_attr "length_immediate" "1")
- (set_attr "prefix_data16" "1,*")
- (set_attr "prefix" "orig,vex")
+ (set_attr "prefix" "maybe_evex")
(set_attr "mode" "<sseinsnmode>")])
-(define_expand "vec_shr_<mode>"
- [(set (match_dup 3)
- (lshiftrt:V1TI
- (match_operand:VI_128 1 "register_operand")
- (match_operand:SI 2 "const_0_to_255_mul_8_operand")))
- (set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
- "TARGET_SSE2"
-{
- operands[1] = gen_lowpart (V1TImode, operands[1]);
- operands[3] = gen_reg_rtx (V1TImode);
- operands[4] = gen_lowpart (<MODE>mode, operands[3]);
-})
+(define_code_attr atom_shift_unit [(ashift "*") (lshiftrt "sishuf")])
-(define_insn "<sse2_avx2>_lshr<mode>3"
+(define_insn "<sse2_avx2>_<shift_insn><mode>3"
[(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
- (lshiftrt:VIMAX_AVX2
+ (any_lshift:VIMAX_AVX2
(match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
(match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
"TARGET_SSE2"
@@ -10842,9 +10827,9 @@ (define_insn "<sse2_avx2>_lshr<mode>3"
switch (which_alternative)
{
case 0:
- return "psrldq\t{%2, %0|%0, %2}";
+ return "p<vshift>dq\t{%2, %0|%0, %2}";
case 1:
- return "vpsrldq\t{%2, %1, %0|%0, %1, %2}";
+ return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
default:
gcc_unreachable ();
}
@@ -10852,7 +10837,7 @@ (define_insn "<sse2_avx2>_lshr<mode>3"
[(set_attr "isa" "noavx,avx")
(set_attr "type" "sseishft")
(set_attr "length_immediate" "1")
- (set_attr "atom_unit" "sishuf")
+ (set_attr "atom_unit" "<atom_shift_unit>")
(set_attr "prefix_data16" "1,*")
(set_attr "prefix" "orig,vex")
(set_attr "mode" "<sseinsnmode>")])
--- gcc/testsuite/gcc.target/i386/pr82370.c.jj 2017-10-24 16:22:16.665464886 +0200
+++ gcc/testsuite/gcc.target/i386/pr82370.c 2017-10-24 16:22:16.665464886 +0200
@@ -0,0 +1,18 @@
+/* PR target/82370 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -mavx512vl -mavx512bw -masm=att" } */
+/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
+/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
+
+#include <x86intrin.h>
+
+__m512i f1 (__m512i *x) { return _mm512_bslli_epi128 (*x, 5); }
+__m512i f2 (__m512i *x) { return _mm512_bsrli_epi128 (*x, 5); }
+__m256i f3 (__m256i *x) { return _mm256_bslli_epi128 (*x, 5); }
+__m256i f4 (__m256i *x) { return _mm256_bsrli_epi128 (*x, 5); }
+__m128i f5 (__m128i *x) { return _mm_bslli_si128 (*x, 5); }
+__m128i f6 (__m128i *x) { return _mm_bsrli_si128 (*x, 5); }
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve V?TImode shifts (PR target/82370)
2017-10-24 14:54 ` Jakub Jelinek
@ 2017-10-24 17:35 ` Uros Bizjak
2017-10-24 19:12 ` Kirill Yukhin
0 siblings, 1 reply; 7+ messages in thread
From: Uros Bizjak @ 2017-10-24 17:35 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: H.J. Lu, Kirill Yukhin, GCC Patches
On Tue, Oct 24, 2017 at 4:46 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Oct 24, 2017 at 05:44:44AM -0700, H.J. Lu wrote:
>> > What I can see from config/atom.md:
>> > ;; if palignr or psrldq
>> > (define_insn_reservation "atom_sseishft_2" 1
>> > (and (eq_attr "cpu" "atom")
>> > (and (eq_attr "type" "sseishft")
>> > (and (eq_attr "atom_unit" "sishuf")
>> > (match_operand 2 "immediate_operand"))))
>> > "atom-simple-0")
>> >
>> > This leads back to initial commit of atom.md.
>> > So, discrimination of psrldq and pslldq looks intentional.
>> >
>> > On the over hand, I see in Software Optimization Guide, Table 14-2 that
>> > PSRLDQ and PSLLDQ occupy same line which directs both insns to port-0 (p 14-18).
>> > So, looking from that point, definition for PSLLDQ which allow either of port-0
>> > and port-1 looks wrong (atom-simple-either reservation).
>> >
>> > In absence of other information, I'd play on safe side and leave things as they
>> > occur right now.
>> >
>>
>> I prefer to leave atom.md ASIS. As for (set_attr "atom_unit"
>> "sishuf"), it was added
>> for
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44615
>>
>> You can drop (set_attr "atom_unit" "sishuf") if gcc.target/i386/sse2-vec-2a.c
>> still compiles.
>
> No, it was added earlier than that, that PR was about insns with psrldq with
> implicit immediate (which don't have a CONST_INT operands[2]). This insn
> does have it, the testcase passes regardless of whether sishuf or other is
> used, it is purely a tuning thing.
>
> In any case, here is an updated patch that just preserves the status quo
> (psrldq having the sishuf unit, pslldq not) using a simple code attribute.
Agner Fog's tables confirm Jakub's observation:
PSLL/RL/RAW/D/Q (x)mm,(x)mm 2 FP0 5 5
PSLL/RL/RAW/D/Q (x)xmm,i 1 FP0 1 1
PSLL/RLDQ xmm,i 1 FP0 1 1
I fail to see how could left and right shifts use different units.
Since the test passes, let's change pslldq to use sishuf unit. There
is no better alternative from the list of units.
> 2017-10-24 Jakub Jelinek <jakub@redhat.com>
>
> PR target/82370
> * config/i386/sse.md (VIMAX_AVX2): Remove V4TImode.
> (VIMAX_AVX2_AVX512BW, VIMAX_AVX512VL): New mode iterators.
> (vec_shl_<mode>): Remove unused expander.
> (avx512bw_<shift_insn><mode>3): New define_insn.
> (atom_shift_unit): New code iterator.
> (<sse2_avx2>_ashl<mode>3, <sse2_avx2>_lshr<mode>3): Replaced by ...
> (<sse2_avx2>_<shift_insn><mode>3): ... this. New define_insn.
>
> * gcc.target/i386/pr82370.c: New test.
OK with the change od pslldq's unit to sishuf.
Thanks,
Uros.
> --- gcc/config/i386/sse.md.jj 2017-10-20 16:30:35.286208652 +0200
> +++ gcc/config/i386/sse.md 2017-10-24 16:29:54.848934888 +0200
> @@ -371,10 +371,17 @@ (define_mode_iterator V16FI
> [V16SF V16SI])
>
> ;; ??? We should probably use TImode instead.
> -(define_mode_iterator VIMAX_AVX2
> +(define_mode_iterator VIMAX_AVX2_AVX512BW
> [(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") V1TI])
>
> -;; ??? This should probably be dropped in favor of VIMAX_AVX2.
> +;; Suppose TARGET_AVX512BW as baseline
> +(define_mode_iterator VIMAX_AVX512VL
> + [V4TI (V2TI "TARGET_AVX512VL") (V1TI "TARGET_AVX512VL")])
> +
> +(define_mode_iterator VIMAX_AVX2
> + [(V2TI "TARGET_AVX2") V1TI])
> +
> +;; ??? This should probably be dropped in favor of VIMAX_AVX2_AVX512BW.
> (define_mode_iterator SSESCALARMODE
> [(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") TI])
>
> @@ -10778,9 +10785,9 @@ (define_insn "<shift_insn><mode>3<mask_n
> (set_attr "mode" "<sseinsnmode>")])
>
>
> -(define_expand "vec_shl_<mode>"
> +(define_expand "vec_shr_<mode>"
> [(set (match_dup 3)
> - (ashift:V1TI
> + (lshiftrt:V1TI
> (match_operand:VI_128 1 "register_operand")
> (match_operand:SI 2 "const_0_to_255_mul_8_operand")))
> (set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
> @@ -10791,48 +10798,26 @@ (define_expand "vec_shl_<mode>"
> operands[4] = gen_lowpart (<MODE>mode, operands[3]);
> })
>
> -(define_insn "<sse2_avx2>_ashl<mode>3"
> - [(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
> - (ashift:VIMAX_AVX2
> - (match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
> - (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
> - "TARGET_SSE2"
> +(define_insn "avx512bw_<shift_insn><mode>3"
> + [(set (match_operand:VIMAX_AVX512VL 0 "register_operand" "=v")
> + (any_lshift:VIMAX_AVX512VL
> + (match_operand:VIMAX_AVX512VL 1 "nonimmediate_operand" "vm")
> + (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n")))]
> + "TARGET_AVX512BW"
> {
> operands[2] = GEN_INT (INTVAL (operands[2]) / 8);
> -
> - switch (which_alternative)
> - {
> - case 0:
> - return "pslldq\t{%2, %0|%0, %2}";
> - case 1:
> - return "vpslldq\t{%2, %1, %0|%0, %1, %2}";
> - default:
> - gcc_unreachable ();
> - }
> + return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
> }
> - [(set_attr "isa" "noavx,avx")
> - (set_attr "type" "sseishft")
> + [(set_attr "type" "sseishft")
> (set_attr "length_immediate" "1")
> - (set_attr "prefix_data16" "1,*")
> - (set_attr "prefix" "orig,vex")
> + (set_attr "prefix" "maybe_evex")
> (set_attr "mode" "<sseinsnmode>")])
>
> -(define_expand "vec_shr_<mode>"
> - [(set (match_dup 3)
> - (lshiftrt:V1TI
> - (match_operand:VI_128 1 "register_operand")
> - (match_operand:SI 2 "const_0_to_255_mul_8_operand")))
> - (set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
> - "TARGET_SSE2"
> -{
> - operands[1] = gen_lowpart (V1TImode, operands[1]);
> - operands[3] = gen_reg_rtx (V1TImode);
> - operands[4] = gen_lowpart (<MODE>mode, operands[3]);
> -})
> +(define_code_attr atom_shift_unit [(ashift "*") (lshiftrt "sishuf")])
>
> -(define_insn "<sse2_avx2>_lshr<mode>3"
> +(define_insn "<sse2_avx2>_<shift_insn><mode>3"
> [(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
> - (lshiftrt:VIMAX_AVX2
> + (any_lshift:VIMAX_AVX2
> (match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
> (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
> "TARGET_SSE2"
> @@ -10842,9 +10827,9 @@ (define_insn "<sse2_avx2>_lshr<mode>3"
> switch (which_alternative)
> {
> case 0:
> - return "psrldq\t{%2, %0|%0, %2}";
> + return "p<vshift>dq\t{%2, %0|%0, %2}";
> case 1:
> - return "vpsrldq\t{%2, %1, %0|%0, %1, %2}";
> + return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
> default:
> gcc_unreachable ();
> }
> @@ -10852,7 +10837,7 @@ (define_insn "<sse2_avx2>_lshr<mode>3"
> [(set_attr "isa" "noavx,avx")
> (set_attr "type" "sseishft")
> (set_attr "length_immediate" "1")
> - (set_attr "atom_unit" "sishuf")
> + (set_attr "atom_unit" "<atom_shift_unit>")
> (set_attr "prefix_data16" "1,*")
> (set_attr "prefix" "orig,vex")
> (set_attr "mode" "<sseinsnmode>")])
> --- gcc/testsuite/gcc.target/i386/pr82370.c.jj 2017-10-24 16:22:16.665464886 +0200
> +++ gcc/testsuite/gcc.target/i386/pr82370.c 2017-10-24 16:22:16.665464886 +0200
> @@ -0,0 +1,18 @@
> +/* PR target/82370 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mavx512vl -mavx512bw -masm=att" } */
> +/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
> +/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
> +
> +#include <x86intrin.h>
> +
> +__m512i f1 (__m512i *x) { return _mm512_bslli_epi128 (*x, 5); }
> +__m512i f2 (__m512i *x) { return _mm512_bsrli_epi128 (*x, 5); }
> +__m256i f3 (__m256i *x) { return _mm256_bslli_epi128 (*x, 5); }
> +__m256i f4 (__m256i *x) { return _mm256_bsrli_epi128 (*x, 5); }
> +__m128i f5 (__m128i *x) { return _mm_bslli_si128 (*x, 5); }
> +__m128i f6 (__m128i *x) { return _mm_bsrli_si128 (*x, 5); }
>
>
> Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve V?TImode shifts (PR target/82370)
2017-10-24 17:35 ` Uros Bizjak
@ 2017-10-24 19:12 ` Kirill Yukhin
0 siblings, 0 replies; 7+ messages in thread
From: Kirill Yukhin @ 2017-10-24 19:12 UTC (permalink / raw)
To: Uros Bizjak; +Cc: Jakub Jelinek, H.J. Lu, GCC Patches
On 24 Oct 19:14, Uros Bizjak wrote:
> On Tue, Oct 24, 2017 at 4:46 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Tue, Oct 24, 2017 at 05:44:44AM -0700, H.J. Lu wrote:
> >> > What I can see from config/atom.md:
> >> > ;; if palignr or psrldq
> >> > (define_insn_reservation "atom_sseishft_2" 1
> >> > (and (eq_attr "cpu" "atom")
> >> > (and (eq_attr "type" "sseishft")
> >> > (and (eq_attr "atom_unit" "sishuf")
> >> > (match_operand 2 "immediate_operand"))))
> >> > "atom-simple-0")
> >> >
> >> > This leads back to initial commit of atom.md.
> >> > So, discrimination of psrldq and pslldq looks intentional.
> >> >
> >> > On the over hand, I see in Software Optimization Guide, Table 14-2 that
> >> > PSRLDQ and PSLLDQ occupy same line which directs both insns to port-0 (p 14-18).
> >> > So, looking from that point, definition for PSLLDQ which allow either of port-0
> >> > and port-1 looks wrong (atom-simple-either reservation).
> >> >
> >> > In absence of other information, I'd play on safe side and leave things as they
> >> > occur right now.
> >> >
> >>
> >> I prefer to leave atom.md ASIS. As for (set_attr "atom_unit"
> >> "sishuf"), it was added
> >> for
> >>
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44615
> >>
> >> You can drop (set_attr "atom_unit" "sishuf") if gcc.target/i386/sse2-vec-2a.c
> >> still compiles.
> >
> > No, it was added earlier than that, that PR was about insns with psrldq with
> > implicit immediate (which don't have a CONST_INT operands[2]). This insn
> > does have it, the testcase passes regardless of whether sishuf or other is
> > used, it is purely a tuning thing.
> >
> > In any case, here is an updated patch that just preserves the status quo
> > (psrldq having the sishuf unit, pslldq not) using a simple code attribute.
>
> Agner Fog's tables confirm Jakub's observation:
>
> PSLL/RL/RAW/D/Q (x)mm,(x)mm 2 FP0 5 5
> PSLL/RL/RAW/D/Q (x)xmm,i 1 FP0 1 1
> PSLL/RLDQ xmm,i 1 FP0 1 1
>
> I fail to see how could left and right shifts use different units.
> Since the test passes, let's change pslldq to use sishuf unit. There
> is no better alternative from the list of units.
Then I bet your patch is OK for main trunk.
--
Thanks, K
>
> > 2017-10-24 Jakub Jelinek <jakub@redhat.com>
> >
> > PR target/82370
> > * config/i386/sse.md (VIMAX_AVX2): Remove V4TImode.
> > (VIMAX_AVX2_AVX512BW, VIMAX_AVX512VL): New mode iterators.
> > (vec_shl_<mode>): Remove unused expander.
> > (avx512bw_<shift_insn><mode>3): New define_insn.
> > (atom_shift_unit): New code iterator.
> > (<sse2_avx2>_ashl<mode>3, <sse2_avx2>_lshr<mode>3): Replaced by ...
> > (<sse2_avx2>_<shift_insn><mode>3): ... this. New define_insn.
> >
> > * gcc.target/i386/pr82370.c: New test.
>
> OK with the change od pslldq's unit to sishuf.
>
> Thanks,
> Uros.
>
> > --- gcc/config/i386/sse.md.jj 2017-10-20 16:30:35.286208652 +0200
> > +++ gcc/config/i386/sse.md 2017-10-24 16:29:54.848934888 +0200
> > @@ -371,10 +371,17 @@ (define_mode_iterator V16FI
> > [V16SF V16SI])
> >
> > ;; ??? We should probably use TImode instead.
> > -(define_mode_iterator VIMAX_AVX2
> > +(define_mode_iterator VIMAX_AVX2_AVX512BW
> > [(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") V1TI])
> >
> > -;; ??? This should probably be dropped in favor of VIMAX_AVX2.
> > +;; Suppose TARGET_AVX512BW as baseline
> > +(define_mode_iterator VIMAX_AVX512VL
> > + [V4TI (V2TI "TARGET_AVX512VL") (V1TI "TARGET_AVX512VL")])
> > +
> > +(define_mode_iterator VIMAX_AVX2
> > + [(V2TI "TARGET_AVX2") V1TI])
> > +
> > +;; ??? This should probably be dropped in favor of VIMAX_AVX2_AVX512BW.
> > (define_mode_iterator SSESCALARMODE
> > [(V4TI "TARGET_AVX512BW") (V2TI "TARGET_AVX2") TI])
> >
> > @@ -10778,9 +10785,9 @@ (define_insn "<shift_insn><mode>3<mask_n
> > (set_attr "mode" "<sseinsnmode>")])
> >
> >
> > -(define_expand "vec_shl_<mode>"
> > +(define_expand "vec_shr_<mode>"
> > [(set (match_dup 3)
> > - (ashift:V1TI
> > + (lshiftrt:V1TI
> > (match_operand:VI_128 1 "register_operand")
> > (match_operand:SI 2 "const_0_to_255_mul_8_operand")))
> > (set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
> > @@ -10791,48 +10798,26 @@ (define_expand "vec_shl_<mode>"
> > operands[4] = gen_lowpart (<MODE>mode, operands[3]);
> > })
> >
> > -(define_insn "<sse2_avx2>_ashl<mode>3"
> > - [(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
> > - (ashift:VIMAX_AVX2
> > - (match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
> > - (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
> > - "TARGET_SSE2"
> > +(define_insn "avx512bw_<shift_insn><mode>3"
> > + [(set (match_operand:VIMAX_AVX512VL 0 "register_operand" "=v")
> > + (any_lshift:VIMAX_AVX512VL
> > + (match_operand:VIMAX_AVX512VL 1 "nonimmediate_operand" "vm")
> > + (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n")))]
> > + "TARGET_AVX512BW"
> > {
> > operands[2] = GEN_INT (INTVAL (operands[2]) / 8);
> > -
> > - switch (which_alternative)
> > - {
> > - case 0:
> > - return "pslldq\t{%2, %0|%0, %2}";
> > - case 1:
> > - return "vpslldq\t{%2, %1, %0|%0, %1, %2}";
> > - default:
> > - gcc_unreachable ();
> > - }
> > + return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
> > }
> > - [(set_attr "isa" "noavx,avx")
> > - (set_attr "type" "sseishft")
> > + [(set_attr "type" "sseishft")
> > (set_attr "length_immediate" "1")
> > - (set_attr "prefix_data16" "1,*")
> > - (set_attr "prefix" "orig,vex")
> > + (set_attr "prefix" "maybe_evex")
> > (set_attr "mode" "<sseinsnmode>")])
> >
> > -(define_expand "vec_shr_<mode>"
> > - [(set (match_dup 3)
> > - (lshiftrt:V1TI
> > - (match_operand:VI_128 1 "register_operand")
> > - (match_operand:SI 2 "const_0_to_255_mul_8_operand")))
> > - (set (match_operand:VI_128 0 "register_operand") (match_dup 4))]
> > - "TARGET_SSE2"
> > -{
> > - operands[1] = gen_lowpart (V1TImode, operands[1]);
> > - operands[3] = gen_reg_rtx (V1TImode);
> > - operands[4] = gen_lowpart (<MODE>mode, operands[3]);
> > -})
> > +(define_code_attr atom_shift_unit [(ashift "*") (lshiftrt "sishuf")])
> >
> > -(define_insn "<sse2_avx2>_lshr<mode>3"
> > +(define_insn "<sse2_avx2>_<shift_insn><mode>3"
> > [(set (match_operand:VIMAX_AVX2 0 "register_operand" "=x,v")
> > - (lshiftrt:VIMAX_AVX2
> > + (any_lshift:VIMAX_AVX2
> > (match_operand:VIMAX_AVX2 1 "register_operand" "0,v")
> > (match_operand:SI 2 "const_0_to_255_mul_8_operand" "n,n")))]
> > "TARGET_SSE2"
> > @@ -10842,9 +10827,9 @@ (define_insn "<sse2_avx2>_lshr<mode>3"
> > switch (which_alternative)
> > {
> > case 0:
> > - return "psrldq\t{%2, %0|%0, %2}";
> > + return "p<vshift>dq\t{%2, %0|%0, %2}";
> > case 1:
> > - return "vpsrldq\t{%2, %1, %0|%0, %1, %2}";
> > + return "vp<vshift>dq\t{%2, %1, %0|%0, %1, %2}";
> > default:
> > gcc_unreachable ();
> > }
> > @@ -10852,7 +10837,7 @@ (define_insn "<sse2_avx2>_lshr<mode>3"
> > [(set_attr "isa" "noavx,avx")
> > (set_attr "type" "sseishft")
> > (set_attr "length_immediate" "1")
> > - (set_attr "atom_unit" "sishuf")
> > + (set_attr "atom_unit" "<atom_shift_unit>")
> > (set_attr "prefix_data16" "1,*")
> > (set_attr "prefix" "orig,vex")
> > (set_attr "mode" "<sseinsnmode>")])
> > --- gcc/testsuite/gcc.target/i386/pr82370.c.jj 2017-10-24 16:22:16.665464886 +0200
> > +++ gcc/testsuite/gcc.target/i386/pr82370.c 2017-10-24 16:22:16.665464886 +0200
> > @@ -0,0 +1,18 @@
> > +/* PR target/82370 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -mavx512vl -mavx512bw -masm=att" } */
> > +/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
> > +/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %xmm\[0-9]\+" 1 } } */
> > +/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
> > +/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %ymm\[0-9]\+" 1 } } */
> > +/* { dg-final { scan-assembler-times "vpslldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
> > +/* { dg-final { scan-assembler-times "vpsrldq\[ \t]\+\\\$5, \\(%\[a-z0-9,]*\\), %zmm\[0-9]\+" 1 } } */
> > +
> > +#include <x86intrin.h>
> > +
> > +__m512i f1 (__m512i *x) { return _mm512_bslli_epi128 (*x, 5); }
> > +__m512i f2 (__m512i *x) { return _mm512_bsrli_epi128 (*x, 5); }
> > +__m256i f3 (__m256i *x) { return _mm256_bslli_epi128 (*x, 5); }
> > +__m256i f4 (__m256i *x) { return _mm256_bsrli_epi128 (*x, 5); }
> > +__m128i f5 (__m128i *x) { return _mm_bslli_si128 (*x, 5); }
> > +__m128i f6 (__m128i *x) { return _mm_bsrli_si128 (*x, 5); }
> >
> >
> > Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-10-24 19:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-04 19:35 [PATCH] Improve V?TImode shifts (PR target/82370) Jakub Jelinek
2017-10-04 21:23 ` H.J. Lu
2017-10-20 19:48 ` Kirill Yukhin
2017-10-24 12:47 ` H.J. Lu
2017-10-24 14:54 ` Jakub Jelinek
2017-10-24 17:35 ` Uros Bizjak
2017-10-24 19:12 ` Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).