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
