This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH][x86][PR73350][PR80862]


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: 0001-test_rounding.patch
Description: 0001-test_rounding.patch


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]