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: [SVE] [fwprop] PR88833 - Redundant moves for WHILELO-based loops


On Wed, 3 Jul 2019 at 17:06, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > On Tue, 2 Jul 2019 at 18:22, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >> > On Tue, 2 Jul 2019 at 16:59, Richard Sandiford
> >> > <richard.sandiford@arm.com> wrote:
> >> >>
> >> >> Thanks for fixing this.
> >> >>
> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >> >> > diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
> >> >> > index 89a46a933fa..79bd0cfbd28 100644
> >> >> > --- a/gcc/simplify-rtx.c
> >> >> > +++ b/gcc/simplify-rtx.c
> >> >> > @@ -6697,6 +6697,19 @@ simplify_subreg (machine_mode outermode, rtx op,
> >> >> >       }
> >> >> >      }
> >> >> >
> >> >> > +  /* If op is a vector comparison operator, rewrite it in a new mode.
> >> >> > +     This probably won't match, but may allow further simplifications.
> >> >> > +     Also check if number of elements and size of each element
> >> >> > +     match for outermode and innermode.  */
> >> >> > +
> >> >>
> >> >> Excess blank line after the comment.  IMO the second part of the comment
> >> >> reads too much like an afterthought.  How about:
> >> >>
> >> >>   /* If OP is a vector comparison and the subreg is not changing the
> >> >>      number of elements or the size of the elements, change the result
> >> >>      of the comparison to the new mode.  */
> >> >>
> >> >> > +  if (COMPARISON_P (op)
> >> >> > +      && VECTOR_MODE_P (outermode)
> >> >> > +      && VECTOR_MODE_P (innermode)
> >> >> > +      && (known_eq (GET_MODE_NUNITS (outermode), GET_MODE_NUNITS (innermode)))
> >> >> > +      && (known_eq (GET_MODE_UNIT_SIZE (outermode),
> >> >> > +                 GET_MODE_UNIT_SIZE (innermode))))
> >> >>
> >> >> Redundant brackets around the known_eq calls.
> >> >>
> >> >> > +    return gen_rtx_fmt_ee (GET_CODE (op), outermode, XEXP (op, 0), XEXP (op, 1));
> >> >>
> >> >> This should use simplify_gen_relational, so that we try to simplify
> >> >> the new expression.
> >> > Does the attached version look OK ?
> >>
> >> OK with a suitable changelog (remember to post those!) if it passes testing.
> > The change to simplify_subreg regressed avx2-pr54700-1.C -;)
> >
> > For following test-case:
> > __attribute__((noipa)) __v8sf
> > f7 (__v8si a, __v8sf b, __v8sf c)
> > {
> >   return a < 0 ? b : c;
> > }
> >
> > Before patch, combine shows:
> > Trying 8, 9 -> 10:
> >     8: r87:V8SI=const_vector
> >     9: r89:V8SI=r87:V8SI>r90:V8SI
> >       REG_DEAD r90:V8SI
> >       REG_DEAD r87:V8SI
> >    10: r86:V8SF=unspec[r92:V8SF,r91:V8SF,r89:V8SI#0] 104
> >       REG_DEAD r92:V8SF
> >       REG_DEAD r89:V8SI
> >       REG_DEAD r91:V8SF
> > Successfully matched this instruction:
> > (set (reg:V8SF 86)
> >     (unspec:V8SF [
> >             (reg:V8SF 92)
> >             (reg:V8SF 91)
> >             (subreg:V8SF (lt:V8SI (reg:V8SI 90)
> >                     (const_vector:V8SI [
> >                             (const_int 0 [0]) repeated x8
> >                         ])) 0)
> >         ] UNSPEC_BLENDV))
> > allowing combination of insns 8, 9 and 10
> >
> > After applying patch, combine shows:
> >
> > Trying 8, 9 -> 10:
> >     8: r87:V8SI=const_vector
> >     9: r89:V8SI=r87:V8SI>r90:V8SI
> >       REG_DEAD r90:V8SI
> >       REG_DEAD r87:V8SI
> >    10: r86:V8SF=unspec[r92:V8SF,r91:V8SF,r89:V8SI#0] 104
> >       REG_DEAD r92:V8SF
> >       REG_DEAD r89:V8SI
> >       REG_DEAD r91:V8SF
> > Failed to match this instruction:
> > (set (reg:V8SF 86)
> >     (unspec:V8SF [
> >             (reg:V8SF 92)
> >             (reg:V8SF 91)
> >             (lt:V8SF (reg:V8SI 90)
> >                 (const_vector:V8SI [
> >                         (const_int 0 [0]) repeated x8
> >                     ]))
> >         ] UNSPEC_BLENDV))
>
> Bah.  If the port isn't self-consistent about whether a subreg should
> be used, it's tempting to say that a subreg should be used and fix the
> places that don't.  At least that way we'd avoid the abomination -
> ABOMINATION! - of using NaNs to represent truth.
>
> But I agree it looks like this is the only pattern affected.
>
> > Adjusting UNSPEC_BLENDV pattern to match the simplified subreg seems to work.
> > Does it look OK ?
> >
> > Testing the attached patch in progress.
> > (A quick comparison for i386.exp now shows no difference before/after patch).
> >
> > Thanks,
> > Prathamesh
> >>
> >> Thanks,
> >> Richard
> >
> > 2019-07-03  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>
> >
> >       * fwprop.c (reg_single_def_p): New function.
> >       (propagate_rtx_1): Add unconditional else inside RTX_EXTRA case.
> >       (forward_propagate_into): New parameter reg_prop_only
> >       with default value false.
> >       Propagate def's src into loop only if SET_SRC and SET_DEST
> >       of def_set have single definitions.
> >       Likewise if reg_prop_only is set to true.
> >       (fwprop): New param fwprop_addr_p.
> >       Integrate fwprop_addr into fwprop.
> >       (fwprop_addr): Remove.
> >       (pass_rtl_fwprop_addr::execute): Call fwprop with arg set
> >       to true.
> >       (pass_rtl_fwprop::execute): Call fwprop with arg set to false.
> >       * simplify-rtx.c (simplify_subreg): Add case for vector comparison.
> >       * i386/config/sse.md (UNSPEC_BLENDV): Adjust pattern.
>
> typo: config/i386/sse.md
>
> >
> > testsuite/
> >       * gfortran.dg/pr88833.f90: New test.
> >
> > diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md
> > index d7d542524fb..5384fe218f9 100644
> > --- a/gcc/config/i386/sse.md
> > +++ b/gcc/config/i386/sse.md
> > @@ -16623,10 +16623,9 @@
> >       (unspec:VF_128_256
> >         [(match_operand:VF_128_256 1 "register_operand" "0,0,x")
> >          (match_operand:VF_128_256 2 "vector_operand" "YrBm,*xBm,xm")
> > -        (subreg:VF_128_256
> > -          (lt:<sseintvecmode>
> > +          (lt:VF_128_256
> >              (match_operand:<sseintvecmode> 3 "register_operand" "Yz,Yz,x")
> > -            (match_operand:<sseintvecmode> 4 "const0_operand" "C,C,C")) 0)]
> > +            (match_operand:<sseintvecmode> 4 "const0_operand" "C,C,C"))]
> >         UNSPEC_BLENDV))]
> >    "TARGET_SSE4_1"
> >    "#"
>
> The (lt:...) and its operands should now be indented by two columns fewer
> than previously.
>
> OK with that change, thanks.
Thanks, committed in r273040.

Thanks,
Prathamesh
>
> Richard


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