* [PATCH][x86][PR73350][PR80862] @ 2017-05-26 9:30 Koval, Julia 2017-05-30 7:13 ` [PATCH][x86][PR73350][PR80862] Uros Bizjak 2017-05-31 8:48 ` [PATCH][x86][PR73350][PR80862] Kirill Yukhin 0 siblings, 2 replies; 6+ messages in thread From: Koval, Julia @ 2017-05-26 9:30 UTC (permalink / raw) To: GCC Patches; +Cc: Uros Bizjak, Peryt, Sebastian [-- Attachment #1: Type: text/plain, Size: 229 bytes --] Hi, This patch fixes these PR's. Ok for trunk? gcc/ * config/i386/subst.md (round): Fix round pattern. * config/i386/i386.c (ix86_erase_embedded_rounding): Fix erasing rounding for the fixed pattern. Thanks, Julia [-- Attachment #2: 0001-patch_1.patch --] [-- Type: application/octet-stream, Size: 3350 bytes --] From 41c2f87eaae29f2d3fcea3e8253308cad058dcee Mon Sep 17 00:00:00 2001 From: "julia.koval" <jkoval@gkliclel110.igk.intel.com> Date: Fri, 26 May 2017 00:07:59 +0300 Subject: [PATCH] patch_1 --- gcc/config/i386/i386.c | 64 ++++++++++++++++++++++++++++-------------------- gcc/config/i386/subst.md | 10 ++++---- 2 files changed, 42 insertions(+), 32 deletions(-) diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 6413aa3..707351f 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -36483,38 +36483,48 @@ ix86_erase_embedded_rounding (rtx pat) if (GET_CODE (pat) == INSN) pat = PATTERN (pat); - gcc_assert (GET_CODE (pat) == PARALLEL); + if (GET_CODE (pat) == PARALLEL) + { + if (XVECLEN (pat, 0) == 2) + { + rtx p0 = XVECEXP (pat, 0, 0); + rtx p1 = XVECEXP (pat, 0, 1); + gcc_assert (GET_CODE (p0) == SET + && GET_CODE (p1) == UNSPEC + && XINT (p1, 1) == UNSPEC_EMBEDDED_ROUNDING); + return p0; + } + else + { + rtx *res = XALLOCAVEC (rtx, XVECLEN (pat, 0)); + int i = 0; + int j = 0; - if (XVECLEN (pat, 0) == 2) - { - rtx p0 = XVECEXP (pat, 0, 0); - rtx p1 = XVECEXP (pat, 0, 1); + for (; i < XVECLEN (pat, 0); ++i) + { + rtx elem = XVECEXP (pat, 0, i); + if (GET_CODE (elem) != UNSPEC + || XINT (elem, 1) != UNSPEC_EMBEDDED_ROUNDING) + res[j++] = elem; + } - gcc_assert (GET_CODE (p0) == SET - && GET_CODE (p1) == UNSPEC - && XINT (p1, 1) == UNSPEC_EMBEDDED_ROUNDING); + /* No more than 1 occurence was removed. */ + gcc_assert (j >= XVECLEN (pat, 0) - 1); - return p0; + return gen_rtx_PARALLEL (GET_MODE (pat), gen_rtvec_v (j, res)); } + } else - { - rtx *res = XALLOCAVEC (rtx, XVECLEN (pat, 0)); - int i = 0; - int j = 0; - - for (; i < XVECLEN (pat, 0); ++i) - { - rtx elem = XVECEXP (pat, 0, i); - if (GET_CODE (elem) != UNSPEC - || XINT (elem, 1) != UNSPEC_EMBEDDED_ROUNDING) - res [j++] = elem; - } - - /* No more than 1 occurence was removed. */ - gcc_assert (j >= XVECLEN (pat, 0) - 1); - - return gen_rtx_PARALLEL (GET_MODE (pat), gen_rtvec_v (j, res)); - } + { + gcc_assert (GET_CODE (pat) == SET); + rtx src = SET_SRC (pat); + gcc_assert (XVECLEN (src, 0) == 2); + rtx p0 = XVECEXP (src, 0, 0); + gcc_assert (GET_CODE (src) == UNSPEC + && XINT (src, 1) == UNSPEC_EMBEDDED_ROUNDING); + rtx res = gen_rtx_SET (SET_DEST (pat), p0); + return res; + } } /* Subroutine of ix86_expand_round_builtin to take care of comi insns diff --git a/gcc/config/i386/subst.md b/gcc/config/i386/subst.md index 0bc22fd..2e632b9 100644 --- a/gcc/config/i386/subst.md +++ b/gcc/config/i386/subst.md @@ -137,12 +137,12 @@ (define_subst "round" [(set (match_operand:SUBST_A 0) - (match_operand:SUBST_A 1))] + (match_operand:SUBST_A 1))] "TARGET_AVX512F" - [(parallel[ - (set (match_dup 0) - (match_dup 1)) - (unspec [(match_operand:SI 2 "const_4_or_8_to_11_operand")] UNSPEC_EMBEDDED_ROUNDING)])]) + [(set (match_dup 0) + (unspec:SUBST_A [(match_dup 1) + (match_operand:SI 2 "const_4_or_8_to_11_operand")] + UNSPEC_EMBEDDED_ROUNDING))]) (define_subst_attr "round_saeonly_name" "round_saeonly" "" "_round") (define_subst_attr "round_saeonly_mask_operand2" "mask" "%r2" "%r4") -- 2.5.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][x86][PR73350][PR80862] 2017-05-26 9:30 [PATCH][x86][PR73350][PR80862] Koval, Julia @ 2017-05-30 7:13 ` Uros Bizjak 2017-05-31 8:48 ` [PATCH][x86][PR73350][PR80862] Kirill Yukhin 1 sibling, 0 replies; 6+ messages in thread From: Uros Bizjak @ 2017-05-30 7:13 UTC (permalink / raw) To: Koval, Julia; +Cc: GCC Patches, Peryt, Sebastian, Kirill Yukhin On Fri, May 26, 2017 at 11:13 AM, Koval, Julia <julia.koval@intel.com> wrote: > Hi, > This patch fixes these PR's. Ok for trunk? > > gcc/ > * config/i386/subst.md (round): Fix round pattern. > * config/i386/i386.c (ix86_erase_embedded_rounding): > Fix erasing rounding for the fixed pattern. I think that Kirill (CC'd) should review this change. It fundamentally changes embedded rounding feature of AVX512F. Uros. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][x86][PR73350][PR80862] 2017-05-26 9:30 [PATCH][x86][PR73350][PR80862] Koval, Julia 2017-05-30 7:13 ` [PATCH][x86][PR73350][PR80862] Uros Bizjak @ 2017-05-31 8:48 ` Kirill Yukhin 2017-05-31 10:34 ` [PATCH][x86][PR73350][PR80862] Kirill Yukhin 1 sibling, 1 reply; 6+ messages in thread From: Kirill Yukhin @ 2017-05-31 8:48 UTC (permalink / raw) To: Koval, Julia Cc: GCC Patches, Uros Bizjak, Peryt, Sebastian, jakub, richard.guenther Hello Julia, On 26 May 09:13, Koval, Julia wrote: > Hi, > This patch fixes these PR's. Ok for trunk? > > gcc/ > * config/i386/subst.md (round): Fix round pattern. > * config/i386/i386.c (ix86_erase_embedded_rounding): > Fix erasing rounding for the fixed pattern. > > Thanks, > Julia Let me copy-paste parts of your patch here. diff --git a/gcc/config/i386/subst.md b/gcc/config/i386/subst.md index 0bc22fd..2e632b9 100644 --- a/gcc/config/i386/subst.md +++ b/gcc/config/i386/subst.md @@ -137,12 +137,12 @@ (define_subst "round" [(set (match_operand:SUBST_A 0) - (match_operand:SUBST_A 1))] + (match_operand:SUBST_A 1))] "TARGET_AVX512F" - [(parallel[ - (set (match_dup 0) - (match_dup 1)) - (unspec [(match_operand:SI 2 "const_4_or_8_to_11_operand")] UNSPEC_EMBEDDED_ROUNDING)])]) + [(set (match_dup 0) + (unspec:SUBST_A [(match_dup 1) + (match_operand:SI 2 "const_4_or_8_to_11_operand")] + UNSPEC_EMBEDDED_ROUNDING))]) (define_subst_attr "round_saeonly_name" "round_saeonly" "" "_round") (define_subst_attr "round_saeonly_mask_operand2" "mask" "%r2" "%r4") -- 2.5.5 So, you propose to put RC as third argument to the set expression. I am not sure that we might use (set ...) in such a way. GCC Internals state that: (set lval x) Represents the action of storing the value of x into the place represented by lval. May this lead to problems? From the first looks like answer is NO and we're might add stuff back to the set expression. Jakub, Richard, could you pls comment on this? -- Thanks, K ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][x86][PR73350][PR80862] 2017-05-31 8:48 ` [PATCH][x86][PR73350][PR80862] Kirill Yukhin @ 2017-05-31 10:34 ` Kirill Yukhin 2017-06-05 10:13 ` [PATCH][x86][PR73350][PR80862] Koval, Julia 0 siblings, 1 reply; 6+ messages in thread From: Kirill Yukhin @ 2017-05-31 10:34 UTC (permalink / raw) To: Koval, Julia Cc: GCC Patches, Uros Bizjak, Peryt, Sebastian, jakub, richard.guenther On 31 May 11:38, Kirill Yukhin wrote: > Hello Julia, > On 26 May 09:13, Koval, Julia wrote: > > Hi, > > This patch fixes these PR's. Ok for trunk? > > > > gcc/ > > * config/i386/subst.md (round): Fix round pattern. > > * config/i386/i386.c (ix86_erase_embedded_rounding): > > Fix erasing rounding for the fixed pattern. > > > > Thanks, > > Julia > > Let me copy-paste parts of your patch here. > diff --git a/gcc/config/i386/subst.md b/gcc/config/i386/subst.md > index 0bc22fd..2e632b9 100644 > --- a/gcc/config/i386/subst.md > +++ b/gcc/config/i386/subst.md > @@ -137,12 +137,12 @@ > > (define_subst "round" > [(set (match_operand:SUBST_A 0) > - (match_operand:SUBST_A 1))] > + (match_operand:SUBST_A 1))] > "TARGET_AVX512F" > - [(parallel[ > - (set (match_dup 0) > - (match_dup 1)) > - (unspec [(match_operand:SI 2 "const_4_or_8_to_11_operand")] UNSPEC_EMBEDDED_ROUNDING)])]) > + [(set (match_dup 0) > + (unspec:SUBST_A [(match_dup 1) > + (match_operand:SI 2 "const_4_or_8_to_11_operand")] > + UNSPEC_EMBEDDED_ROUNDING))]) > > (define_subst_attr "round_saeonly_name" "round_saeonly" "" "_round") > (define_subst_attr "round_saeonly_mask_operand2" "mask" "%r2" "%r4") > -- > 2.5.5 > > So, you propose to put RC as third argument to the set expression. > I am not sure that we might use (set ...) in such a way. Whoops, I was wrong. You are setting w/ SET_SRC as UNSPEC which which is eliminated conditionally, which is much better to me. Few nits: 1. diff --git a/gcc/config/i386/subst.md b/gcc/config/i386/subst.md index 0bc22fd..2e632b9 100644 --- a/gcc/config/i386/subst.md +++ b/gcc/config/i386/subst.md @@ -137,12 +137,12 @@ (define_subst "round" [(set (match_operand:SUBST_A 0) - (match_operand:SUBST_A 1))] + (match_operand:SUBST_A 1))] "TARGET_AVX512F" Junk. 2. Check identation pls 3. Mention PR in ChangeLog entry 4. Add reg test please Overall I like this approach. We must somehow set explicit dependency between RC and actual op, why not this way? Could you pls make sure that CSE is still working for ops w/ identical RC? To be paranoid: is it possible to check skylake-avx512 w/ and w/o the patch on Spec2k6? -- Thanks, K ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH][x86][PR73350][PR80862] 2017-05-31 10:34 ` [PATCH][x86][PR73350][PR80862] Kirill Yukhin @ 2017-06-05 10:13 ` Koval, Julia 2017-06-08 11:26 ` [PATCH][x86][PR73350][PR80862] Kirill Yukhin 0 siblings, 1 reply; 6+ messages in thread From: Koval, Julia @ 2017-06-05 10:13 UTC (permalink / raw) To: Kirill Yukhin Cc: GCC Patches, Uros Bizjak, Peryt, Sebastian, jakub, richard.guenther [-- Attachment #1: Type: text/plain, Size: 3284 bytes --] Hi, 1 is replace 8 spaces with tab suggested by ./check_GNU_style.sh, should I still fix it back? 2,3,4 Done CSE is working, spec 2k6 on skylake-avx512 has same score. PR target/73350,80862 gcc/ * config/i386/subst.md (round): Fix round pattern. * config/i386/i386.c (ix86_erase_embedded_rounding): Fix erasing rounding for the fixed pattern. gcc/testsuite * gcc.target/i386/pr73350.c: New test. Ok for trunk? Thanks, Julia > -----Original Message----- > From: Kirill Yukhin [mailto:kirill.yukhin@gmail.com] > Sent: Wednesday, May 31, 2017 12:34 PM > To: Koval, Julia <julia.koval@intel.com> > Cc: GCC Patches <gcc-patches@gcc.gnu.org>; Uros Bizjak > <ubizjak@gmail.com>; Peryt, Sebastian <sebastian.peryt@intel.com>; > jakub@redhat.com; richard.guenther@gmail.com > Subject: Re: [PATCH][x86][PR73350][PR80862] > > On 31 May 11:38, Kirill Yukhin wrote: > > Hello Julia, > > On 26 May 09:13, Koval, Julia wrote: > > > Hi, > > > This patch fixes these PR's. Ok for trunk? > > > > > > gcc/ > > > * config/i386/subst.md (round): Fix round pattern. > > > * config/i386/i386.c (ix86_erase_embedded_rounding): > > > Fix erasing rounding for the fixed pattern. > > > > > > Thanks, > > > Julia > > > > Let me copy-paste parts of your patch here. > > diff --git a/gcc/config/i386/subst.md b/gcc/config/i386/subst.md > > index 0bc22fd..2e632b9 100644 > > --- a/gcc/config/i386/subst.md > > +++ b/gcc/config/i386/subst.md > > @@ -137,12 +137,12 @@ > > > > (define_subst "round" > > [(set (match_operand:SUBST_A 0) > > - (match_operand:SUBST_A 1))] > > + (match_operand:SUBST_A 1))] > > "TARGET_AVX512F" > > - [(parallel[ > > - (set (match_dup 0) > > - (match_dup 1)) > > - (unspec [(match_operand:SI 2 "const_4_or_8_to_11_operand")] > UNSPEC_EMBEDDED_ROUNDING)])]) > > + [(set (match_dup 0) > > + (unspec:SUBST_A [(match_dup 1) > > + (match_operand:SI 2 "const_4_or_8_to_11_operand")] > > + UNSPEC_EMBEDDED_ROUNDING))]) > > > > (define_subst_attr "round_saeonly_name" "round_saeonly" "" "_round") > > (define_subst_attr "round_saeonly_mask_operand2" "mask" "%r2" "%r4") > > -- > > 2.5.5 > > > > So, you propose to put RC as third argument to the set expression. > > I am not sure that we might use (set ...) in such a way. > Whoops, I was wrong. You are setting w/ SET_SRC as UNSPEC which > which is eliminated conditionally, which is much better to me. > > Few nits: > 1. > diff --git a/gcc/config/i386/subst.md b/gcc/config/i386/subst.md > index 0bc22fd..2e632b9 100644 > --- a/gcc/config/i386/subst.md > +++ b/gcc/config/i386/subst.md > @@ -137,12 +137,12 @@ > > (define_subst "round" > [(set (match_operand:SUBST_A 0) > - (match_operand:SUBST_A 1))] > + (match_operand:SUBST_A 1))] > "TARGET_AVX512F" > Junk. > > 2. Check identation pls > > 3. Mention PR in ChangeLog entry > > 4. Add reg test please > > Overall I like this approach. We must somehow set explicit dependency > between RC and actual op, why not this way? > > Could you pls make sure that CSE is still working for ops w/ identical RC? > > To be paranoid: is it possible to check skylake-avx512 w/ and w/o the patch > on Spec2k6? > > -- > Thanks, K [-- Attachment #2: 0001-test_rounding.patch --] [-- Type: application/octet-stream, Size: 4234 bytes --] From 1a219bef2e7fe92b18ab487dec59ab5a8905901a Mon Sep 17 00:00:00 2001 From: "julia.koval" <jkoval@gkliclel110.igk.intel.com> Date: Mon, 5 Jun 2017 10:47:42 +0300 Subject: [PATCH] test_rounding --- gcc/config/i386/i386.c | 64 +++++++++++++++++++-------------- gcc/config/i386/subst.md | 11 +++--- gcc/testsuite/gcc.target/i386/pr73350.c | 19 ++++++++++ 3 files changed, 62 insertions(+), 32 deletions(-) create mode 100644 gcc/testsuite/gcc.target/i386/pr73350.c diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 6413aa3..707351f 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -36483,38 +36483,48 @@ ix86_erase_embedded_rounding (rtx pat) if (GET_CODE (pat) == INSN) pat = PATTERN (pat); - gcc_assert (GET_CODE (pat) == PARALLEL); + if (GET_CODE (pat) == PARALLEL) + { + if (XVECLEN (pat, 0) == 2) + { + rtx p0 = XVECEXP (pat, 0, 0); + rtx p1 = XVECEXP (pat, 0, 1); + gcc_assert (GET_CODE (p0) == SET + && GET_CODE (p1) == UNSPEC + && XINT (p1, 1) == UNSPEC_EMBEDDED_ROUNDING); + return p0; + } + else + { + rtx *res = XALLOCAVEC (rtx, XVECLEN (pat, 0)); + int i = 0; + int j = 0; - if (XVECLEN (pat, 0) == 2) - { - rtx p0 = XVECEXP (pat, 0, 0); - rtx p1 = XVECEXP (pat, 0, 1); + for (; i < XVECLEN (pat, 0); ++i) + { + rtx elem = XVECEXP (pat, 0, i); + if (GET_CODE (elem) != UNSPEC + || XINT (elem, 1) != UNSPEC_EMBEDDED_ROUNDING) + res[j++] = elem; + } - gcc_assert (GET_CODE (p0) == SET - && GET_CODE (p1) == UNSPEC - && XINT (p1, 1) == UNSPEC_EMBEDDED_ROUNDING); + /* No more than 1 occurence was removed. */ + gcc_assert (j >= XVECLEN (pat, 0) - 1); - return p0; + return gen_rtx_PARALLEL (GET_MODE (pat), gen_rtvec_v (j, res)); } + } else - { - rtx *res = XALLOCAVEC (rtx, XVECLEN (pat, 0)); - int i = 0; - int j = 0; - - for (; i < XVECLEN (pat, 0); ++i) - { - rtx elem = XVECEXP (pat, 0, i); - if (GET_CODE (elem) != UNSPEC - || XINT (elem, 1) != UNSPEC_EMBEDDED_ROUNDING) - res [j++] = elem; - } - - /* No more than 1 occurence was removed. */ - gcc_assert (j >= XVECLEN (pat, 0) - 1); - - return gen_rtx_PARALLEL (GET_MODE (pat), gen_rtvec_v (j, res)); - } + { + gcc_assert (GET_CODE (pat) == SET); + rtx src = SET_SRC (pat); + gcc_assert (XVECLEN (src, 0) == 2); + rtx p0 = XVECEXP (src, 0, 0); + gcc_assert (GET_CODE (src) == UNSPEC + && XINT (src, 1) == UNSPEC_EMBEDDED_ROUNDING); + rtx res = gen_rtx_SET (SET_DEST (pat), p0); + return res; + } } /* Subroutine of ix86_expand_round_builtin to take care of comi insns diff --git a/gcc/config/i386/subst.md b/gcc/config/i386/subst.md index 0bc22fd..57fb0d4 100644 --- a/gcc/config/i386/subst.md +++ b/gcc/config/i386/subst.md @@ -137,12 +137,13 @@ (define_subst "round" [(set (match_operand:SUBST_A 0) - (match_operand:SUBST_A 1))] + (match_operand:SUBST_A 1))] "TARGET_AVX512F" - [(parallel[ - (set (match_dup 0) - (match_dup 1)) - (unspec [(match_operand:SI 2 "const_4_or_8_to_11_operand")] UNSPEC_EMBEDDED_ROUNDING)])]) + [(set (match_dup 0) + (unspec:SUBST_A [(match_dup 1) + (match_operand:SI 2 "const_4_or_8_to_11_operand")] + UNSPEC_EMBEDDED_ROUNDING)) +]) (define_subst_attr "round_saeonly_name" "round_saeonly" "" "_round") (define_subst_attr "round_saeonly_mask_operand2" "mask" "%r2" "%r4") diff --git a/gcc/testsuite/gcc.target/i386/pr73350.c b/gcc/testsuite/gcc.target/i386/pr73350.c new file mode 100644 index 0000000..62f6cd4 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr73350.c @@ -0,0 +1,19 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -mavx512f" } */ +/* { dg-require-effective-target avx512f } */ +#include <math.h> +#define AVX512F +#include "avx512f-helper.h" + +void +TEST (void) +{ + __m512 a = _mm512_set1_ps ((float) M_PI); + __m512 b = _mm512_set1_ps ((float) 1.f); + + __m512 result1 = _mm512_add_round_ps (a, b, (_MM_FROUND_TO_NEG_INF | _MM_FROUND_NO_EXC)); + __m512 result2 = _mm512_add_round_ps (a, b, (_MM_FROUND_TO_POS_INF | _MM_FROUND_NO_EXC)); + + if (result1[0] == result2[0]) + abort (); +} -- 2.5.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][x86][PR73350][PR80862] 2017-06-05 10:13 ` [PATCH][x86][PR73350][PR80862] Koval, Julia @ 2017-06-08 11:26 ` Kirill Yukhin 0 siblings, 0 replies; 6+ messages in thread From: Kirill Yukhin @ 2017-06-08 11:26 UTC (permalink / raw) To: Koval, Julia Cc: GCC Patches, Uros Bizjak, Peryt, Sebastian, jakub, richard.guenther Hello Julia, On 05 Jun 10:13, Koval, Julia wrote: > Hi, > > 1 is replace 8 spaces with tab suggested by ./check_GNU_style.sh, should I still fix it back? > 2,3,4 Done Thanks a lot! Your patch is OK for trunk. I've checked it in for you (r249009.). > CSE is working, spec 2k6 on skylake-avx512 has same score. Great. -- Thanks, K ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-08 11:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-26 9:30 [PATCH][x86][PR73350][PR80862] Koval, Julia 2017-05-30 7:13 ` [PATCH][x86][PR73350][PR80862] Uros Bizjak 2017-05-31 8:48 ` [PATCH][x86][PR73350][PR80862] Kirill Yukhin 2017-05-31 10:34 ` [PATCH][x86][PR73350][PR80862] Kirill Yukhin 2017-06-05 10:13 ` [PATCH][x86][PR73350][PR80862] Koval, Julia 2017-06-08 11:26 ` [PATCH][x86][PR73350][PR80862] 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).