[2/2] validate_subreg before call gen_lowpart to avoid ICE.

Message ID 20210910125818.334531-3-hongtao.liu@intel.com
State New
Headers show
Series
  • Revert r12-3277 since it caused regressions on many other targets.
Related show

Commit Message

Jakub Jelinek via Gcc-patches Sept. 10, 2021, 12:58 p.m.
gcc/ChangeLog:

	* expmed.c (extract_bit_field_using_extv): validate_subreg
	before call gen_lowpart.
---
 gcc/expmed.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.27.0

Comments

Jakub Jelinek via Gcc-patches Sept. 10, 2021, 1:15 p.m. | #1
On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:
>

> gcc/ChangeLog:

>

>         * expmed.c (extract_bit_field_using_extv): validate_subreg

>         before call gen_lowpart.

> ---

>  gcc/expmed.c | 6 +++++-

>  1 file changed, 5 insertions(+), 1 deletion(-)

>

> diff --git a/gcc/expmed.c b/gcc/expmed.c

> index 3143f38e057..10d62d857a8 100644

> --- a/gcc/expmed.c

> +++ b/gcc/expmed.c

> @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

>

>    if (GET_MODE (target) != ext_mode)

>      {

> +      machine_mode tmode = GET_MODE (target);

>        /* Don't use LHS paradoxical subreg if explicit truncation is needed

>          between the mode of the extraction (word_mode) and the target

>          mode.  Instead, create a temporary and use convert_move to set

>          the target.  */

>        if (REG_P (target)

> -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))


^^^

I wonder if herein lies the problem in that the HFmode "truncation" from SImode
is considered noop?  Note the underlying target hook only looks at the mode
precision and thus receives 16 and 32, and thus maybe that
TRULY_NOOP_TRUNCATION_MODES_P query only makes sense for
integer modes?  Though the documentation of the hook only talks about
"conversion" of "values" ...

So maybe a targetm.modes_tieable_p (GET_MODE (target), extmode) check
is missing?

> +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> +         && validate_subreg (ext_mode, tmode,

> +                             target,

> +                             subreg_lowpart_offset (ext_mode, tmode)))

>         {

>           target = gen_lowpart (ext_mode, target);

>           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> --

> 2.27.0

>
Jakub Jelinek via Gcc-patches Sept. 10, 2021, 1:27 p.m. | #2
On Fri, Sep 10, 2021 at 9:16 PM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>

> On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> >

> > gcc/ChangeLog:

> >

> >         * expmed.c (extract_bit_field_using_extv): validate_subreg

> >         before call gen_lowpart.

> > ---

> >  gcc/expmed.c | 6 +++++-

> >  1 file changed, 5 insertions(+), 1 deletion(-)

> >

> > diff --git a/gcc/expmed.c b/gcc/expmed.c

> > index 3143f38e057..10d62d857a8 100644

> > --- a/gcc/expmed.c

> > +++ b/gcc/expmed.c

> > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

> >

> >    if (GET_MODE (target) != ext_mode)

> >      {

> > +      machine_mode tmode = GET_MODE (target);

> >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

> >          between the mode of the extraction (word_mode) and the target

> >          mode.  Instead, create a temporary and use convert_move to set

> >          the target.  */

> >        if (REG_P (target)

> > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

>

> ^^^

>

> I wonder if herein lies the problem in that the HFmode "truncation" from SImode

> is considered noop?  Note the underlying target hook only looks at the mode

> precision and thus receives 16 and 32, and thus maybe that

> TRULY_NOOP_TRUNCATION_MODES_P query only makes sense for

> integer modes?  Though the documentation of the hook only talks about

> "conversion" of "values" ...

>

> So maybe a targetm.modes_tieable_p (GET_MODE (target), extmode) check

> is missing?


According to document, it should be true for
targetm.modes_tieable_p(HFmode, SImode) since HFmode can be allocated
to gpr.

----------------
This hook returns true if a value of mode mode1 is accessible in mode
mode2 without
copying
-------------------

and also here gen_lowpart (SImode, HFmode, target) is called and hit
gcc_assert, not (subreg:HF (reg:SI) 0)

>

> > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> > +         && validate_subreg (ext_mode, tmode,

> > +                             target,

> > +                             subreg_lowpart_offset (ext_mode, tmode)))

> >         {

> >           target = gen_lowpart (ext_mode, target);

> >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> > --

> > 2.27.0

> >




-- 
BR,
Hongtao
Segher Boessenkool Sept. 10, 2021, 1:30 p.m. | #3
On Fri, Sep 10, 2021 at 03:15:56PM +0200, Richard Biener wrote:
> On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> >        if (REG_P (target)

> > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> 

> ^^^

> 

> I wonder if herein lies the problem in that the HFmode "truncation" from SImode

> is considered noop?  Note the underlying target hook only looks at the mode

> precision and thus receives 16 and 32, and thus maybe that

> TRULY_NOOP_TRUNCATION_MODES_P query only makes sense for

> integer modes?  Though the documentation of the hook only talks about

> "conversion" of "values" ...


@deftypefn {Target Hook} bool TARGET_TRULY_NOOP_TRUNCATION (poly_uint64 @var{outprec}, poly_uint64 @var{inprec})
This hook returns true if it is safe to ``convert'' a value of
@var{inprec} bits to one of @var{outprec} bits (where @var{outprec} is
smaller than @var{inprec}) by merely operating on it as if it had only
@var{outprec} bits.  The default returns true unconditionally, which
is correct for most machines.  When @code{TARGET_TRULY_NOOP_TRUNCATION}
returns false, the machine description should provide a @code{trunc}
optab to specify the RTL that performs the required truncation.


@cindex @code{trunc@var{m}@var{n}2} instruction pattern
@item @samp{trunc@var{m}@var{n}2}
Truncate operand 1 (valid for mode @var{m}) to mode @var{n} and
store in operand 0 (which has mode @var{n}).  Both modes must be fixed
point or both floating point.


TRULY_NOOP_TRUNCATION does not make sense to ask if changing mode class.


Segher
Jakub Jelinek via Gcc-patches Sept. 10, 2021, 1:32 p.m. | #4
On September 10, 2021 3:27:09 PM GMT+02:00, Hongtao Liu <crazylht@gmail.com> wrote:
>On Fri, Sep 10, 2021 at 9:16 PM Richard Biener via Gcc-patches

><gcc-patches@gcc.gnu.org> wrote:

>>

>> On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

>> >

>> > gcc/ChangeLog:

>> >

>> >         * expmed.c (extract_bit_field_using_extv): validate_subreg

>> >         before call gen_lowpart.

>> > ---

>> >  gcc/expmed.c | 6 +++++-

>> >  1 file changed, 5 insertions(+), 1 deletion(-)

>> >

>> > diff --git a/gcc/expmed.c b/gcc/expmed.c

>> > index 3143f38e057..10d62d857a8 100644

>> > --- a/gcc/expmed.c

>> > +++ b/gcc/expmed.c

>> > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

>> >

>> >    if (GET_MODE (target) != ext_mode)

>> >      {

>> > +      machine_mode tmode = GET_MODE (target);

>> >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

>> >          between the mode of the extraction (word_mode) and the target

>> >          mode.  Instead, create a temporary and use convert_move to set

>> >          the target.  */

>> >        if (REG_P (target)

>> > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

>>

>> ^^^

>>

>> I wonder if herein lies the problem in that the HFmode "truncation" from SImode

>> is considered noop?  Note the underlying target hook only looks at the mode

>> precision and thus receives 16 and 32, and thus maybe that

>> TRULY_NOOP_TRUNCATION_MODES_P query only makes sense for

>> integer modes?  Though the documentation of the hook only talks about

>> "conversion" of "values" ...

>>

>> So maybe a targetm.modes_tieable_p (GET_MODE (target), extmode) check

>> is missing?

>

>According to document, it should be true for

>targetm.modes_tieable_p(HFmode, SImode) since HFmode can be allocated

>to gpr.

>

>----------------

>This hook returns true if a value of mode mode1 is accessible in mode

>mode2 without

>copying

>-------------------

>

>and also here gen_lowpart (SImode, HFmode, target) is called and hit

>gcc_assert, not (subreg:HF (reg:SI) 0)


I see. Of course that leads to a suggestion to allow the subreg based on modes_tieable_p, but then others will know why that's the wrong thing to do? 

Richard. 

>>

>> > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

>> > +         && validate_subreg (ext_mode, tmode,

>> > +                             target,

>> > +                             subreg_lowpart_offset (ext_mode, tmode)))

>> >         {

>> >           target = gen_lowpart (ext_mode, target);

>> >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

>> > --

>> > 2.27.0

>> >

>

>

>
Jakub Jelinek via Gcc-patches Sept. 10, 2021, 1:39 p.m. | #5
On Fri, Sep 10, 2021 at 9:27 PM Hongtao Liu <crazylht@gmail.com> wrote:
>

> On Fri, Sep 10, 2021 at 9:16 PM Richard Biener via Gcc-patches

> <gcc-patches@gcc.gnu.org> wrote:

> >

> > On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> > >

> > > gcc/ChangeLog:

> > >

> > >         * expmed.c (extract_bit_field_using_extv): validate_subreg

> > >         before call gen_lowpart.

> > > ---

> > >  gcc/expmed.c | 6 +++++-

> > >  1 file changed, 5 insertions(+), 1 deletion(-)

> > >

> > > diff --git a/gcc/expmed.c b/gcc/expmed.c

> > > index 3143f38e057..10d62d857a8 100644

> > > --- a/gcc/expmed.c

> > > +++ b/gcc/expmed.c

> > > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

> > >

> > >    if (GET_MODE (target) != ext_mode)

> > >      {

> > > +      machine_mode tmode = GET_MODE (target);

> > >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

> > >          between the mode of the extraction (word_mode) and the target

> > >          mode.  Instead, create a temporary and use convert_move to set

> > >          the target.  */

> > >        if (REG_P (target)

> > > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> >

> > ^^^

> >

> > I wonder if herein lies the problem in that the HFmode "truncation" from SImode

> > is considered noop?  Note the underlying target hook only looks at the mode

> > precision and thus receives 16 and 32, and thus maybe that

> > TRULY_NOOP_TRUNCATION_MODES_P query only makes sense for

> > integer modes?  Though the documentation of the hook only talks about

> > "conversion" of "values" ...

> >

> > So maybe a targetm.modes_tieable_p (GET_MODE (target), extmode) check

> > is missing?

>

> According to document, it should be true for

> targetm.modes_tieable_p(HFmode, SImode) since HFmode can be allocated

> to gpr.

I was wrong it needs *any* r, so targetm.modes_tieable_p do return false here.

If TARGET_HARD_REGNO_MODE_OK (r, mode1) and TARGET_HARD_REGNO_MODE_OK (r,
mode2) are always the same for any r, then TARGET_MODES_TIEABLE_P (mode1,
mode2) should be true. If they differ for any r, you should define this hook to
return false unless some other mechanism ensures the accessibility of
the value in a
narrower mode.
You should define this hook to return true in as many cases as
possible since doing so
will allow GCC to perform better register allocation. The default
definition returns
true unconditionally.

>

> ----------------

> This hook returns true if a value of mode mode1 is accessible in mode

> mode2 without

> copying

> -------------------

>

> and also here gen_lowpart (SImode, HFmode, target) is called and hit

> gcc_assert, not (subreg:HF (reg:SI) 0)

>

> >

> > > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> > > +         && validate_subreg (ext_mode, tmode,

> > > +                             target,

> > > +                             subreg_lowpart_offset (ext_mode, tmode)))

> > >         {

> > >           target = gen_lowpart (ext_mode, target);

> > >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> > > --

> > > 2.27.0

> > >

>

>

>

> --

> BR,

> Hongtao




-- 
BR,
Hongtao
Jakub Jelinek via Gcc-patches Sept. 10, 2021, 1:44 p.m. | #6
On Fri, Sep 10, 2021 at 9:32 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On September 10, 2021 3:27:09 PM GMT+02:00, Hongtao Liu <crazylht@gmail.com> wrote:

> >On Fri, Sep 10, 2021 at 9:16 PM Richard Biener via Gcc-patches

> ><gcc-patches@gcc.gnu.org> wrote:

> >>

> >> On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> >> >

> >> > gcc/ChangeLog:

> >> >

> >> >         * expmed.c (extract_bit_field_using_extv): validate_subreg

> >> >         before call gen_lowpart.

> >> > ---

> >> >  gcc/expmed.c | 6 +++++-

> >> >  1 file changed, 5 insertions(+), 1 deletion(-)

> >> >

> >> > diff --git a/gcc/expmed.c b/gcc/expmed.c

> >> > index 3143f38e057..10d62d857a8 100644

> >> > --- a/gcc/expmed.c

> >> > +++ b/gcc/expmed.c

> >> > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

> >> >

> >> >    if (GET_MODE (target) != ext_mode)

> >> >      {

> >> > +      machine_mode tmode = GET_MODE (target);

> >> >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

> >> >          between the mode of the extraction (word_mode) and the target

> >> >          mode.  Instead, create a temporary and use convert_move to set

> >> >          the target.  */

> >> >        if (REG_P (target)

> >> > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> >>

> >> ^^^

> >>

> >> I wonder if herein lies the problem in that the HFmode "truncation" from SImode

> >> is considered noop?  Note the underlying target hook only looks at the mode

> >> precision and thus receives 16 and 32, and thus maybe that

> >> TRULY_NOOP_TRUNCATION_MODES_P query only makes sense for

> >> integer modes?  Though the documentation of the hook only talks about

> >> "conversion" of "values" ...

> >>

> >> So maybe a targetm.modes_tieable_p (GET_MODE (target), extmode) check

> >> is missing?

> >

> >According to document, it should be true for

> >targetm.modes_tieable_p(HFmode, SImode) since HFmode can be allocated

> >to gpr.

> >

> >----------------

> >This hook returns true if a value of mode mode1 is accessible in mode

> >mode2 without

> >copying

> >-------------------

> >

> >and also here gen_lowpart (SImode, HFmode, target) is called and hit

> >gcc_assert, not (subreg:HF (reg:SI) 0)

>

> I see. Of course that leads to a suggestion to allow the subreg based on modes_tieable_p, but then others will know why that's the wrong thing to do?

I'm testing this

1 file changed, 2 insertions(+), 1 deletion(-)
gcc/expmed.c | 3 ++-

modified   gcc/expmed.c
@@ -1576,7 +1576,8 @@ extract_bit_field_using_extv (const
extraction_insn *extv, rtx op0,
  mode.  Instead, create a temporary and use convert_move to set
  the target.  */
       if (REG_P (target)
        -   && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))
        +   && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode)
        +   && targetm.modes_tieable_p (GET_MODE (target), ext_mode))
  {
    target = gen_lowpart (ext_mode, target);
    if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

>

> Richard.

>

> >>

> >> > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> >> > +         && validate_subreg (ext_mode, tmode,

> >> > +                             target,

> >> > +                             subreg_lowpart_offset (ext_mode, tmode)))

> >> >         {

> >> >           target = gen_lowpart (ext_mode, target);

> >> >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> >> > --

> >> > 2.27.0

> >> >

> >

> >

> >

>



-- 
BR,
Hongtao
Jakub Jelinek via Gcc-patches Sept. 10, 2021, 1:52 p.m. | #7
On Fri, Sep 10, 2021 at 9:32 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On September 10, 2021 3:27:09 PM GMT+02:00, Hongtao Liu <crazylht@gmail.com> wrote:

> >On Fri, Sep 10, 2021 at 9:16 PM Richard Biener via Gcc-patches

> ><gcc-patches@gcc.gnu.org> wrote:

> >>

> >> On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> >> >

> >> > gcc/ChangeLog:

> >> >

> >> >         * expmed.c (extract_bit_field_using_extv): validate_subreg

> >> >         before call gen_lowpart.

> >> > ---

> >> >  gcc/expmed.c | 6 +++++-

> >> >  1 file changed, 5 insertions(+), 1 deletion(-)

> >> >

> >> > diff --git a/gcc/expmed.c b/gcc/expmed.c

> >> > index 3143f38e057..10d62d857a8 100644

> >> > --- a/gcc/expmed.c

> >> > +++ b/gcc/expmed.c

> >> > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

> >> >

> >> >    if (GET_MODE (target) != ext_mode)

> >> >      {

> >> > +      machine_mode tmode = GET_MODE (target);

> >> >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

> >> >          between the mode of the extraction (word_mode) and the target

> >> >          mode.  Instead, create a temporary and use convert_move to set

> >> >          the target.  */

> >> >        if (REG_P (target)

> >> > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> >>

> >> ^^^

> >>

> >> I wonder if herein lies the problem in that the HFmode "truncation" from SImode

> >> is considered noop?  Note the underlying target hook only looks at the mode

> >> precision and thus receives 16 and 32, and thus maybe that

> >> TRULY_NOOP_TRUNCATION_MODES_P query only makes sense for

> >> integer modes?  Though the documentation of the hook only talks about

> >> "conversion" of "values" ...

> >>

> >> So maybe a targetm.modes_tieable_p (GET_MODE (target), extmode) check

> >> is missing?

> >

> >According to document, it should be true for

> >targetm.modes_tieable_p(HFmode, SImode) since HFmode can be allocated

> >to gpr.

> >

> >----------------

> >This hook returns true if a value of mode mode1 is accessible in mode

> >mode2 without

> >copying

> >-------------------

> >

> >and also here gen_lowpart (SImode, HFmode, target) is called and hit

> >gcc_assert, not (subreg:HF (reg:SI) 0)

>

> I see. Of course that leads to a suggestion to allow the subreg based on modes_tieable_p, but then others will know why that's the wrong thing to do?

allowing subreg based on modes_tieable_p would be too strict, it even
disallows (subreg SF (reg:SI).
>

> Richard.

>

> >>

> >> > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> >> > +         && validate_subreg (ext_mode, tmode,

> >> > +                             target,

> >> > +                             subreg_lowpart_offset (ext_mode, tmode)))

> >> >         {

> >> >           target = gen_lowpart (ext_mode, target);

> >> >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> >> > --

> >> > 2.27.0

> >> >

> >

> >

> >

>



-- 
BR,
Hongtao
Jakub Jelinek via Gcc-patches Sept. 10, 2021, 1:58 p.m. | #8
On September 10, 2021 3:30:10 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:
>On Fri, Sep 10, 2021 at 03:15:56PM +0200, Richard Biener wrote:

>> On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

>> >        if (REG_P (target)

>> > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

>> 

>> ^^^

>> 

>> I wonder if herein lies the problem in that the HFmode "truncation" from SImode

>> is considered noop?  Note the underlying target hook only looks at the mode

>> precision and thus receives 16 and 32, and thus maybe that

>> TRULY_NOOP_TRUNCATION_MODES_P query only makes sense for

>> integer modes?  Though the documentation of the hook only talks about

>> "conversion" of "values" ...

>

>@deftypefn {Target Hook} bool TARGET_TRULY_NOOP_TRUNCATION (poly_uint64 @var{outprec}, poly_uint64 @var{inprec})

>This hook returns true if it is safe to ``convert'' a value of

>@var{inprec} bits to one of @var{outprec} bits (where @var{outprec} is

>smaller than @var{inprec}) by merely operating on it as if it had only

>@var{outprec} bits.  The default returns true unconditionally, which

>is correct for most machines.  When @code{TARGET_TRULY_NOOP_TRUNCATION}

>returns false, the machine description should provide a @code{trunc}

>optab to specify the RTL that performs the required truncation.

>

>

>@cindex @code{trunc@var{m}@var{n}2} instruction pattern

>@item @samp{trunc@var{m}@var{n}2}

>Truncate operand 1 (valid for mode @var{m}) to mode @var{n} and

>store in operand 0 (which has mode @var{n}).  Both modes must be fixed

>point or both floating point.

>

>

>TRULY_NOOP_TRUNCATION does not make sense to ask if changing mode class.


OK, so there's a mode class comparison missing here which should be a better fix than calling validate_subreg? 

Richard. 

>

>Segher
Jakub Jelinek via Gcc-patches Sept. 10, 2021, 2:25 p.m. | #9
On Fri, Sep 10, 2021 at 9:44 PM Hongtao Liu <crazylht@gmail.com> wrote:
>

> On Fri, Sep 10, 2021 at 9:32 PM Richard Biener

> <richard.guenther@gmail.com> wrote:

> >

> > On September 10, 2021 3:27:09 PM GMT+02:00, Hongtao Liu <crazylht@gmail.com> wrote:

> > >On Fri, Sep 10, 2021 at 9:16 PM Richard Biener via Gcc-patches

> > ><gcc-patches@gcc.gnu.org> wrote:

> > >>

> > >> On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> > >> >

> > >> > gcc/ChangeLog:

> > >> >

> > >> >         * expmed.c (extract_bit_field_using_extv): validate_subreg

> > >> >         before call gen_lowpart.

> > >> > ---

> > >> >  gcc/expmed.c | 6 +++++-

> > >> >  1 file changed, 5 insertions(+), 1 deletion(-)

> > >> >

> > >> > diff --git a/gcc/expmed.c b/gcc/expmed.c

> > >> > index 3143f38e057..10d62d857a8 100644

> > >> > --- a/gcc/expmed.c

> > >> > +++ b/gcc/expmed.c

> > >> > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

> > >> >

> > >> >    if (GET_MODE (target) != ext_mode)

> > >> >      {

> > >> > +      machine_mode tmode = GET_MODE (target);

> > >> >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

> > >> >          between the mode of the extraction (word_mode) and the target

> > >> >          mode.  Instead, create a temporary and use convert_move to set

> > >> >          the target.  */

> > >> >        if (REG_P (target)

> > >> > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> > >>

> > >> ^^^

> > >>

> > >> I wonder if herein lies the problem in that the HFmode "truncation" from SImode

> > >> is considered noop?  Note the underlying target hook only looks at the mode

> > >> precision and thus receives 16 and 32, and thus maybe that

> > >> TRULY_NOOP_TRUNCATION_MODES_P query only makes sense for

> > >> integer modes?  Though the documentation of the hook only talks about

> > >> "conversion" of "values" ...

> > >>

> > >> So maybe a targetm.modes_tieable_p (GET_MODE (target), extmode) check

> > >> is missing?

> > >

> > >According to document, it should be true for

> > >targetm.modes_tieable_p(HFmode, SImode) since HFmode can be allocated

> > >to gpr.

> > >

> > >----------------

> > >This hook returns true if a value of mode mode1 is accessible in mode

> > >mode2 without

> > >copying

> > >-------------------

> > >

> > >and also here gen_lowpart (SImode, HFmode, target) is called and hit

> > >gcc_assert, not (subreg:HF (reg:SI) 0)

> >

> > I see. Of course that leads to a suggestion to allow the subreg based on modes_tieable_p, but then others will know why that's the wrong thing to do?

> I'm testing this

>

> 1 file changed, 2 insertions(+), 1 deletion(-)

> gcc/expmed.c | 3 ++-

>

> modified   gcc/expmed.c

> @@ -1576,7 +1576,8 @@ extract_bit_field_using_extv (const

> extraction_insn *extv, rtx op0,

>   mode.  Instead, create a temporary and use convert_move to set

>   the target.  */

>        if (REG_P (target)

>         -   && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

>         +   && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode)

>         +   && targetm.modes_tieable_p (GET_MODE (target), ext_mode))

>   {

>     target = gen_lowpart (ext_mode, target);

>     if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

>

Updated patch.

  Bootstrapped and regtested on x86_64-linux-gnu{-m32,},  do I need to
run this patch on other targets machine, or the patch is supposed to
have minimal impact on other targets?
  Then, ok for trunk?

> >

> > Richard.

> >

> > >>

> > >> > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> > >> > +         && validate_subreg (ext_mode, tmode,

> > >> > +                             target,

> > >> > +                             subreg_lowpart_offset (ext_mode, tmode)))

> > >> >         {

> > >> >           target = gen_lowpart (ext_mode, target);

> > >> >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> > >> > --

> > >> > 2.27.0

> > >> >

> > >

> > >

> > >

> >

>

>

> --

> BR,

> Hongtao




-- 
BR,
Hongtao
Segher Boessenkool Sept. 10, 2021, 4:24 p.m. | #10
On Fri, Sep 10, 2021 at 03:58:47PM +0200, Richard Biener wrote:
> On September 10, 2021 3:30:10 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> >TRULY_NOOP_TRUNCATION does not make sense to ask if changing mode class.

> 

> OK, so there's a mode class comparison missing here which should be a better fix than calling validate_subreg? 


Yes, we should not call TRULY_NOOP_TRUNCATION_MODES_P for any random two
modes: such a truncation needs to have a meaning at all, for the
question to make any sense.  Maybe we can add an assert to this macro to
root out nonsensical callers?

Btw.  We have
#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2) \
  (targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1), \
                                  GET_MODE_PRECISION (MODE2)))
which is not optimal, either: does truncating DFmode to HFmode behave
the same as truncating DImode to HImode, on every target?  On *any*
target, even?!


Segher
Jakub Jelinek via Gcc-patches Sept. 10, 2021, 6:36 p.m. | #11
On September 10, 2021 6:24:50 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:
>On Fri, Sep 10, 2021 at 03:58:47PM +0200, Richard Biener wrote:

>> On September 10, 2021 3:30:10 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:

>> >TRULY_NOOP_TRUNCATION does not make sense to ask if changing mode class.

>> 

>> OK, so there's a mode class comparison missing here which should be a better fix than calling validate_subreg? 

>

>Yes, we should not call TRULY_NOOP_TRUNCATION_MODES_P for any random two

>modes: such a truncation needs to have a meaning at all, for the

>question to make any sense.  Maybe we can add an assert to this macro to

>root out nonsensical callers?

>

>Btw.  We have

>#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2) \

>  (targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1), \

>                                  GET_MODE_PRECISION (MODE2)))

>which is not optimal, either: does truncating DFmode to HFmode behave

>the same as truncating DImode to HImode, on every target?  On *any*

>target, even?!


When is it for any non-scalar integral mode? I suspect this was only meaningful for integer modes from the start. On x86 i387 math any float mode truncation is noop (with not doing actual truncation to inf). 

>

>

>Segher
Segher Boessenkool Sept. 10, 2021, 9:19 p.m. | #12
On Fri, Sep 10, 2021 at 10:25:45PM +0800, Hongtao Liu wrote:
> Updated patch.

> 

>   Bootstrapped and regtested on x86_64-linux-gnu{-m32,},  do I need to

> run this patch on other targets machine, or the patch is supposed to

> have minimal impact on other targets?

>   Then, ok for trunk?


[-- Attachment #2: v2-0001-Check-modes_tieable_p-before-call-gen_lowpart-to-.pat
ch --]
[-- Type: application/octet-stream, Encoding: base64, Size: 1.4K --]

[-- application/octet-stream is unsupported (use 'v' to view this part) --]

Please send your patches inline, or if you *have to* use attachments,
use text attachments.  Without encoding.

<https://gcc.gnu.org/contribute.html#patches>

(It says "strongly discouraged", which means people will put it to the
bottom of the stack of things to look at).


Segher
Segher Boessenkool Sept. 10, 2021, 9:27 p.m. | #13
On Fri, Sep 10, 2021 at 08:36:12PM +0200, Richard Biener wrote:
> On September 10, 2021 6:24:50 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> >Yes, we should not call TRULY_NOOP_TRUNCATION_MODES_P for any random two

> >modes: such a truncation needs to have a meaning at all, for the

> >question to make any sense.  Maybe we can add an assert to this macro to

> >root out nonsensical callers?

> >

> >Btw.  We have

> >#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2) \

> >  (targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1), \

> >                                  GET_MODE_PRECISION (MODE2)))

> >which is not optimal, either: does truncating DFmode to HFmode behave

> >the same as truncating DImode to HImode, on every target?  On *any*

> >target, even?!

> 

> When is it for any non-scalar integral mode? I suspect this was only meaningful for integer modes from the start. On x86 i387 math any float mode truncation is noop (with not doing actual truncation to inf). 


And trunc on floating point modes was added later?  Yeah, sounds like a
good theory.

So we should have an assertion in TNTMP that both modes are integral?
(Scalar of course).


Segher
Jakub Jelinek via Gcc-patches Sept. 11, 2021, 12:29 a.m. | #14
On Sat, Sep 11, 2021 at 5:21 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>

> On Fri, Sep 10, 2021 at 10:25:45PM +0800, Hongtao Liu wrote:

> > Updated patch.

> >

> >   Bootstrapped and regtested on x86_64-linux-gnu{-m32,},  do I need to

> > run this patch on other targets machine, or the patch is supposed to

> > have minimal impact on other targets?

> >   Then, ok for trunk?

>

> [-- Attachment #2: v2-0001-Check-modes_tieable_p-before-call-gen_lowpart-to-.pat

> ch --]

> [-- Type: application/octet-stream, Encoding: base64, Size: 1.4K --]

>

> [-- application/octet-stream is unsupported (use 'v' to view this part) --]

>

> Please send your patches inline, or if you *have to* use attachments,

> use text attachments.  Without encoding.

>

hmm, my local file is
$file 0001-Check-modes_tieable_p-before-call-gen_lowpart-to-avo.patch
--mime-type
0001-Check-modes_tieable_p-before-call-gen_lowpart-to-avo.patch: text/x-diff

Didn't figure out how to let webgmail not change mime type of attachment.
> <https://gcc.gnu.org/contribute.html#patches>

>

> (It says "strongly discouraged", which means people will put it to the

> bottom of the stack of things to look at).

>

>

> Segher


Here is an updated patch.

  Bootstrapped and regtested on x86_64-linux-gnu{-m32,}
  Ok for trunk?

gcc/ChangeLog:

        * machmode.h (TRULY_NOOP_TRUNCATION_MODES_P): Check
        SCALAR_INT_MODE_P for both modes.
---
 gcc/machmode.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gcc/machmode.h b/gcc/machmode.h
index 158351350de..9f95d7d046c 100644
--- a/gcc/machmode.h
+++ b/gcc/machmode.h
@@ -959,9 +959,10 @@ extern scalar_int_mode ptr_mode;
 /* Target-dependent machine mode initialization - in insn-modes.c.  */
 extern void init_adjust_machine_modes (void);

-#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2) \
-  (targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1), \
-                                 GET_MODE_PRECISION (MODE2)))
+#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2)                    \
+  (SCALAR_INT_MODE_P(MODE1) && SCALAR_INT_MODE_P(MODE2)                \
+   && targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1),       \
+                                    GET_MODE_PRECISION (MODE2)))

 /* Return true if MODE is a scalar integer mode that fits in a
    HOST_WIDE_INT.  */

-- 
BR,
Hongtao
Jakub Jelinek via Gcc-patches Sept. 11, 2021, 1:04 a.m. | #15
On Sat, Sep 11, 2021 at 8:29 AM Hongtao Liu <crazylht@gmail.com> wrote:
>

> On Sat, Sep 11, 2021 at 5:21 AM Segher Boessenkool

> <segher@kernel.crashing.org> wrote:

> >

> > On Fri, Sep 10, 2021 at 10:25:45PM +0800, Hongtao Liu wrote:

> > > Updated patch.

> > >

> > >   Bootstrapped and regtested on x86_64-linux-gnu{-m32,},  do I need to

> > > run this patch on other targets machine, or the patch is supposed to

> > > have minimal impact on other targets?

> > >   Then, ok for trunk?

> >

> > [-- Attachment #2: v2-0001-Check-modes_tieable_p-before-call-gen_lowpart-to-.pat

> > ch --]

> > [-- Type: application/octet-stream, Encoding: base64, Size: 1.4K --]

> >

> > [-- application/octet-stream is unsupported (use 'v' to view this part) --]

> >

> > Please send your patches inline, or if you *have to* use attachments,

> > use text attachments.  Without encoding.

> >

> hmm, my local file is

> $file 0001-Check-modes_tieable_p-before-call-gen_lowpart-to-avo.patch

> --mime-type

> 0001-Check-modes_tieable_p-before-call-gen_lowpart-to-avo.patch: text/x-diff

>

> Didn't figure out how to let webgmail not change mime type of attachment.

> > <https://gcc.gnu.org/contribute.html#patches>

> >

> > (It says "strongly discouraged", which means people will put it to the

> > bottom of the stack of things to look at).

> >

> >

> > Segher

>

> Here is an updated patch.

>

>   Bootstrapped and regtested on x86_64-linux-gnu{-m32,}

>   Ok for trunk?

>

> gcc/ChangeLog:

>

>         * machmode.h (TRULY_NOOP_TRUNCATION_MODES_P): Check

>         SCALAR_INT_MODE_P for both modes.

> ---

>  gcc/machmode.h | 7 ++++---

>  1 file changed, 4 insertions(+), 3 deletions(-)

>

> diff --git a/gcc/machmode.h b/gcc/machmode.h

> index 158351350de..9f95d7d046c 100644

> --- a/gcc/machmode.h

> +++ b/gcc/machmode.h

> @@ -959,9 +959,10 @@ extern scalar_int_mode ptr_mode;

>  /* Target-dependent machine mode initialization - in insn-modes.c.  */

>  extern void init_adjust_machine_modes (void);

>

> -#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2) \

> -  (targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1), \

> -                                 GET_MODE_PRECISION (MODE2)))

> +#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2)                    \

> +  (SCALAR_INT_MODE_P(MODE1) && SCALAR_INT_MODE_P(MODE2)                \

will add space for SCALAR_INT_MODE_P (MODE1) && SCALAR_INT_MODE_P (MODE2)
> +   && targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1),       \

> +                                    GET_MODE_PRECISION (MODE2)))

>

>  /* Return true if MODE is a scalar integer mode that fits in a

>     HOST_WIDE_INT.  */

>

> --

> BR,

> Hongtao




-- 
BR,
Hongtao
Jakub Jelinek via Gcc-patches Sept. 11, 2021, 8:25 a.m. | #16
On September 10, 2021 11:27:16 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:
>On Fri, Sep 10, 2021 at 08:36:12PM +0200, Richard Biener wrote:

>> On September 10, 2021 6:24:50 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:

>> >Yes, we should not call TRULY_NOOP_TRUNCATION_MODES_P for any random two

>> >modes: such a truncation needs to have a meaning at all, for the

>> >question to make any sense.  Maybe we can add an assert to this macro to

>> >root out nonsensical callers?

>> >

>> >Btw.  We have

>> >#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2) \

>> >  (targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1), \

>> >                                  GET_MODE_PRECISION (MODE2)))

>> >which is not optimal, either: does truncating DFmode to HFmode behave

>> >the same as truncating DImode to HImode, on every target?  On *any*

>> >target, even?!

>> 

>> When is it for any non-scalar integral mode? I suspect this was only meaningful for integer modes from the start. On x86 i387 math any float mode truncation is noop (with not doing actual truncation to inf). 

>

>And trunc on floating point modes was added later?  Yeah, sounds like a

>good theory.

>

>So we should have an assertion in TNTMP that both modes are integral?

>(Scalar of course).


No, but in the context we are talking about (bitfield extraction) obviously integer truncate is referred to, a FP truncate doesn't make sense here. So either this piece needs to ask for integer modes and then also pun to them or restrict the operation to integer modes. For source int but destination FP there might be other ways to validate whether the target can do this, but TNTMP is not it. 

Richard. 

>

>Segher
Jakub Jelinek via Gcc-patches Sept. 11, 2021, 9:51 a.m. | #17
On Sat, Sep 11, 2021 at 4:25 PM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>

> On September 10, 2021 11:27:16 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> >On Fri, Sep 10, 2021 at 08:36:12PM +0200, Richard Biener wrote:

> >> On September 10, 2021 6:24:50 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> >> >Yes, we should not call TRULY_NOOP_TRUNCATION_MODES_P for any random two

> >> >modes: such a truncation needs to have a meaning at all, for the

> >> >question to make any sense.  Maybe we can add an assert to this macro to

> >> >root out nonsensical callers?

> >> >

> >> >Btw.  We have

> >> >#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2) \

> >> >  (targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1), \

> >> >                                  GET_MODE_PRECISION (MODE2)))

> >> >which is not optimal, either: does truncating DFmode to HFmode behave

> >> >the same as truncating DImode to HImode, on every target?  On *any*

> >> >target, even?!

> >>

> >> When is it for any non-scalar integral mode? I suspect this was only meaningful for integer modes from the start. On x86 i387 math any float mode truncation is noop (with not doing actual truncation to inf).

> >

> >And trunc on floating point modes was added later?  Yeah, sounds like a

> >good theory.

> >

> >So we should have an assertion in TNTMP that both modes are integral?

> >(Scalar of course).

>

> No, but in the context we are talking about (bitfield extraction) obviously integer truncate is referred to, a FP truncate doesn't make sense here. So either this piece needs to ask for integer modes and then also pun to them or restrict the operation to integer modes. For source int but destination FP there might be other ways to validate whether the target can do this, but TNTMP is not it.

There is no contradiction between integer truncate and target is
FLOAT_MODE_P, extv only cares about the bits extraction, look at code
below, there's convert_extracted_bit_field which is supposed to handle
non scalar integral mode.
The key here is the paradoxical subreg (subreg:ext_mode (target)) is
not necessarily legal, and it is problematic to call gen_lowpart
directly without guaranteeing this, I still prefer to add a
validate_subreg(extmode, tmode) condition.
>

> Richard.

>

> >

> >Segher

>



-- 
BR,
Hongtao
Jakub Jelinek via Gcc-patches Sept. 11, 2021, 11:09 a.m. | #18
On Sat, Sep 11, 2021 at 5:51 PM Hongtao Liu <crazylht@gmail.com> wrote:
>

> On Sat, Sep 11, 2021 at 4:25 PM Richard Biener via Gcc-patches

> <gcc-patches@gcc.gnu.org> wrote:

> >

> > On September 10, 2021 11:27:16 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> > >On Fri, Sep 10, 2021 at 08:36:12PM +0200, Richard Biener wrote:

> > >> On September 10, 2021 6:24:50 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> > >> >Yes, we should not call TRULY_NOOP_TRUNCATION_MODES_P for any random two

> > >> >modes: such a truncation needs to have a meaning at all, for the

> > >> >question to make any sense.  Maybe we can add an assert to this macro to

> > >> >root out nonsensical callers?

> > >> >

> > >> >Btw.  We have

> > >> >#define TRULY_NOOP_TRUNCATION_MODES_P(MODE1, MODE2) \

> > >> >  (targetm.truly_noop_truncation (GET_MODE_PRECISION (MODE1), \

> > >> >                                  GET_MODE_PRECISION (MODE2)))

> > >> >which is not optimal, either: does truncating DFmode to HFmode behave

> > >> >the same as truncating DImode to HImode, on every target?  On *any*

> > >> >target, even?!

> > >>

> > >> When is it for any non-scalar integral mode? I suspect this was only meaningful for integer modes from the start. On x86 i387 math any float mode truncation is noop (with not doing actual truncation to inf).

> > >

> > >And trunc on floating point modes was added later?  Yeah, sounds like a

> > >good theory.

> > >

> > >So we should have an assertion in TNTMP that both modes are integral?

> > >(Scalar of course).

> >

> > No, but in the context we are talking about (bitfield extraction) obviously integer truncate is referred to, a FP truncate doesn't make sense here. So either this piece needs to ask for integer modes and then also pun to them or restrict the operation to integer modes. For source int but destination FP there might be other ways to validate whether the target can do this, but TNTMP is not it.

> There is no contradiction between integer truncate and target is

> FLOAT_MODE_P, extv only cares about the bits extraction, look at code

> below, there's convert_extracted_bit_field which is supposed to handle

> non scalar integral mode.

> The key here is the paradoxical subreg (subreg:ext_mode (target)) is

> not necessarily legal, and it is problematic to call gen_lowpart

> directly without guaranteeing this, I still prefer to add a

> validate_subreg(extmode, tmode) condition.

It seems aarch64 use bitfield extraction for

_Float16
foo (int a)
{
  union {
    int a;
    _Float16 b;
  }c;
  c.a = a;
  return c.b;
}

(insn 2 4 3 2 (set (reg/v:SI 93 [ a ])
        (reg:SI 0 x0 [ a ]))
"../gcc/intel-innersource/master/gcc/testsuite/gcc.target/i386/float16-5.c":5:1
52 {*movsi_aarch64}
     (nil))
(note 3 2 6 2 NOTE_INSN_FUNCTION_BEG)
(insn 6 3 7 2 (set (subreg:DI (reg:HF 94) 0)
        (zero_extract:DI (subreg:DI (reg/v:SI 93 [ a ]) 0)
            (const_int 16 [0x10])
            (const_int 0 [0])))
"../gcc/intel-innersource/master/gcc/testsuite/gcc.target/i386/float16-5.c":11:11
742 {*extzvdi}
     (nil))
(insn 7 6 11 2 (set (reg:HF 92 [ <retval> ])
        (reg:HF 94))
"../gcc/intel-innersource/master/gcc/testsuite/gcc.target/i386/float16-5.c":11:11
59 {*movhf_aarch64}
     (nil))

> >

> > Richard.

> >

> > >

> > >Segher

> >

>

>

> --

> BR,

> Hongtao




-- 
BR,
Hongtao
Jakub Jelinek via Gcc-patches Sept. 13, 2021, 6:10 a.m. | #19
On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:
>

> gcc/ChangeLog:

>

>         * expmed.c (extract_bit_field_using_extv): validate_subreg

>         before call gen_lowpart.

> ---

>  gcc/expmed.c | 6 +++++-

>  1 file changed, 5 insertions(+), 1 deletion(-)

>

> diff --git a/gcc/expmed.c b/gcc/expmed.c

> index 3143f38e057..10d62d857a8 100644

> --- a/gcc/expmed.c

> +++ b/gcc/expmed.c

> @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

>

>    if (GET_MODE (target) != ext_mode)

>      {

> +      machine_mode tmode = GET_MODE (target);

>        /* Don't use LHS paradoxical subreg if explicit truncation is needed

>          between the mode of the extraction (word_mode) and the target

>          mode.  Instead, create a temporary and use convert_move to set

>          the target.  */

>        if (REG_P (target)

> -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> +         && validate_subreg (ext_mode, tmode,

> +                             target,

> +                             subreg_lowpart_offset (ext_mode, tmode)))

>         {

>           target = gen_lowpart (ext_mode, target);


That would be equivalent to use gen_lowpart_if_possible?

>           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> --

> 2.27.0

>
Jakub Jelinek via Gcc-patches Sept. 13, 2021, 6:32 a.m. | #20
On Mon, Sep 13, 2021 at 2:11 PM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>

> On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> >

> > gcc/ChangeLog:

> >

> >         * expmed.c (extract_bit_field_using_extv): validate_subreg

> >         before call gen_lowpart.

> > ---

> >  gcc/expmed.c | 6 +++++-

> >  1 file changed, 5 insertions(+), 1 deletion(-)

> >

> > diff --git a/gcc/expmed.c b/gcc/expmed.c

> > index 3143f38e057..10d62d857a8 100644

> > --- a/gcc/expmed.c

> > +++ b/gcc/expmed.c

> > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

> >

> >    if (GET_MODE (target) != ext_mode)

> >      {

> > +      machine_mode tmode = GET_MODE (target);

> >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

> >          between the mode of the extraction (word_mode) and the target

> >          mode.  Instead, create a temporary and use convert_move to set

> >          the target.  */

> >        if (REG_P (target)

> > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> > +         && validate_subreg (ext_mode, tmode,

> > +                             target,

> > +                             subreg_lowpart_offset (ext_mode, tmode)))

> >         {

> >           target = gen_lowpart (ext_mode, target);

>

> That would be equivalent to use gen_lowpart_if_possible?

No, target will be changed to NULL_RTX.
But it does avoid ICE since maybe_expand_insn can legitimate operands,
but I doubt it will introduce other bugs since the target has been
changed here.

I think the validate_subreg solution is plain and straightforward,
just like it's done in
r11-7515-g0ad6de3883a1641f7ec0bd9cf56d41fa5b313dae.


>

> >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> > --

> > 2.27.0

> >




--
BR,
Hongtao
Jakub Jelinek via Gcc-patches Sept. 13, 2021, 9:15 a.m. | #21
On Mon, Sep 13, 2021 at 8:26 AM Hongtao Liu <crazylht@gmail.com> wrote:
>

> On Mon, Sep 13, 2021 at 2:11 PM Richard Biener via Gcc-patches

> <gcc-patches@gcc.gnu.org> wrote:

> >

> > On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> > >

> > > gcc/ChangeLog:

> > >

> > >         * expmed.c (extract_bit_field_using_extv): validate_subreg

> > >         before call gen_lowpart.

> > > ---

> > >  gcc/expmed.c | 6 +++++-

> > >  1 file changed, 5 insertions(+), 1 deletion(-)

> > >

> > > diff --git a/gcc/expmed.c b/gcc/expmed.c

> > > index 3143f38e057..10d62d857a8 100644

> > > --- a/gcc/expmed.c

> > > +++ b/gcc/expmed.c

> > > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

> > >

> > >    if (GET_MODE (target) != ext_mode)

> > >      {

> > > +      machine_mode tmode = GET_MODE (target);

> > >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

> > >          between the mode of the extraction (word_mode) and the target

> > >          mode.  Instead, create a temporary and use convert_move to set

> > >          the target.  */

> > >        if (REG_P (target)

> > > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> > > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> > > +         && validate_subreg (ext_mode, tmode,

> > > +                             target,

> > > +                             subreg_lowpart_offset (ext_mode, tmode)))

> > >         {

> > >           target = gen_lowpart (ext_mode, target);

> >

> > That would be equivalent to use gen_lowpart_if_possible?

> No, target will be changed to NULL_RTX.

> But it does avoid ICE since maybe_expand_insn can legitimate operands,

> but I doubt it will introduce other bugs since the target has been

> changed here.

>

> I think the validate_subreg solution is plain and straightforward,

> just like it's done in

> r11-7515-g0ad6de3883a1641f7ec0bd9cf56d41fa5b313dae.


That guards an explicit gen_rtx_SUBREG, here we're using gen_lowpart.
It's not an obvious match to validate gen_lowpart with validate_subreg,
I thought that gen_lowpart_if_possible would be prefered.  You obviously
have to adjust the code, like

  rtx tem;
  if (...
      && (tem = gen_lowpart_if_possible (ext_mode, target))
    {
       target = tem;
...

Richard.

>

> >

> > >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> > > --

> > > 2.27.0

> > >

>

>

>

> --

> BR,

> Hongtao
Jakub Jelinek via Gcc-patches Sept. 13, 2021, 11:14 a.m. | #22
On Mon, Sep 13, 2021 at 5:15 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On Mon, Sep 13, 2021 at 8:26 AM Hongtao Liu <crazylht@gmail.com> wrote:

> >

> > On Mon, Sep 13, 2021 at 2:11 PM Richard Biener via Gcc-patches

> > <gcc-patches@gcc.gnu.org> wrote:

> > >

> > > On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> > > >

> > > > gcc/ChangeLog:

> > > >

> > > >         * expmed.c (extract_bit_field_using_extv): validate_subreg

> > > >         before call gen_lowpart.

> > > > ---

> > > >  gcc/expmed.c | 6 +++++-

> > > >  1 file changed, 5 insertions(+), 1 deletion(-)

> > > >

> > > > diff --git a/gcc/expmed.c b/gcc/expmed.c

> > > > index 3143f38e057..10d62d857a8 100644

> > > > --- a/gcc/expmed.c

> > > > +++ b/gcc/expmed.c

> > > > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

> > > >

> > > >    if (GET_MODE (target) != ext_mode)

> > > >      {

> > > > +      machine_mode tmode = GET_MODE (target);

> > > >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

> > > >          between the mode of the extraction (word_mode) and the target

> > > >          mode.  Instead, create a temporary and use convert_move to set

> > > >          the target.  */

> > > >        if (REG_P (target)

> > > > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> > > > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> > > > +         && validate_subreg (ext_mode, tmode,

> > > > +                             target,

> > > > +                             subreg_lowpart_offset (ext_mode, tmode)))

> > > >         {

> > > >           target = gen_lowpart (ext_mode, target);

> > >

> > > That would be equivalent to use gen_lowpart_if_possible?

> > No, target will be changed to NULL_RTX.

> > But it does avoid ICE since maybe_expand_insn can legitimate operands,

> > but I doubt it will introduce other bugs since the target has been

> > changed here.

> >

> > I think the validate_subreg solution is plain and straightforward,

> > just like it's done in

> > r11-7515-g0ad6de3883a1641f7ec0bd9cf56d41fa5b313dae.

>

> That guards an explicit gen_rtx_SUBREG, here we're using gen_lowpart.

> It's not an obvious match to validate gen_lowpart with validate_subreg,

> I thought that gen_lowpart_if_possible would be prefered.  You obviously

> have to adjust the code, like

>

>   rtx tem;

>   if (...

>       && (tem = gen_lowpart_if_possible (ext_mode, target))

>     {

Yes, update patch

  bootstrapped and regtested on x86_64-linux-gnu{-m32,}.
  Ok for trunk?
gcc/ChangeLog:

        * expmed.c (extract_bit_field_using_extv): Use
        gen_lowpart_if_possible instead of gen_lowpart to avoid ICE.
---
 gcc/expmed.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gcc/expmed.c b/gcc/expmed.c
index 3143f38e057..59734d4841c 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -1571,14 +1571,16 @@ extract_bit_field_using_extv (const
extraction_insn *extv, rtx op0,

   if (GET_MODE (target) != ext_mode)
     {
+      rtx temp;
       /* Don't use LHS paradoxical subreg if explicit truncation is needed
         between the mode of the extraction (word_mode) and the target
         mode.  Instead, create a temporary and use convert_move to set
         the target.  */
       if (REG_P (target)
-         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))
+         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode)
+         && (temp = gen_lowpart_if_possible (ext_mode, target)))
        {
-         target = gen_lowpart (ext_mode, target);
+         target = temp;
          if (partial_subreg_p (GET_MODE (spec_target), ext_mode))
            spec_target_subreg = target;
        }
--
2.27.0

>        target = tem;

> ...

>

> Richard.

>

> >

> > >

> > > >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> > > > --

> > > > 2.27.0

> > > >

> >

> >

> >

> > --

> > BR,

> > Hongtao




-- 
BR,
Hongtao
Tobias Burnus Sept. 13, 2021, 11:44 a.m. | #23
On 13.09.21 13:14, Hongtao Liu via Gcc-patches wrote:
> Yes, update patch

>    bootstrapped and regtested on x86_64-linux-gnu{-m32,}.

>    Ok for trunk?

If that patch gets approved, can you add PR bootstrap/102302 to the
commit changelog ?

cf. https://gcc.gnu.org/PR102302

Thanks,

Tobias

> gcc/ChangeLog:

>

>          * expmed.c (extract_bit_field_using_extv): Use

>          gen_lowpart_if_possible instead of gen_lowpart to avoid ICE.

> ---

>   gcc/expmed.c | 6 ++++--

>   1 file changed, 4 insertions(+), 2 deletions(-)

>

> diff --git a/gcc/expmed.c b/gcc/expmed.c

> index 3143f38e057..59734d4841c 100644

> --- a/gcc/expmed.c

> +++ b/gcc/expmed.c

> @@ -1571,14 +1571,16 @@ extract_bit_field_using_extv (const

> extraction_insn *extv, rtx op0,

>

>     if (GET_MODE (target) != ext_mode)

>       {

> +      rtx temp;

>         /* Don't use LHS paradoxical subreg if explicit truncation is needed

>           between the mode of the extraction (word_mode) and the target

>           mode.  Instead, create a temporary and use convert_move to set

>           the target.  */

>         if (REG_P (target)

> -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> +         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode)

> +         && (temp = gen_lowpart_if_possible (ext_mode, target)))

>          {

> -         target = gen_lowpart (ext_mode, target);

> +         target = temp;

>            if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

>              spec_target_subreg = target;

>          }

> --

> 2.27.0

>

>>         target = tem;

>> ...

>>

>> Richard.

>>

>>>>>            if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

>>>>> --

>>>>> 2.27.0

>>>>>

>>>

>>>

>>> --

>>> BR,

>>> Hongtao

>

>

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
Jakub Jelinek via Gcc-patches Sept. 13, 2021, 11:45 a.m. | #24
On Mon, Sep 13, 2021 at 1:14 PM Hongtao Liu <crazylht@gmail.com> wrote:
>

> On Mon, Sep 13, 2021 at 5:15 PM Richard Biener

> <richard.guenther@gmail.com> wrote:

> >

> > On Mon, Sep 13, 2021 at 8:26 AM Hongtao Liu <crazylht@gmail.com> wrote:

> > >

> > > On Mon, Sep 13, 2021 at 2:11 PM Richard Biener via Gcc-patches

> > > <gcc-patches@gcc.gnu.org> wrote:

> > > >

> > > > On Fri, Sep 10, 2021 at 2:58 PM liuhongt <hongtao.liu@intel.com> wrote:

> > > > >

> > > > > gcc/ChangeLog:

> > > > >

> > > > >         * expmed.c (extract_bit_field_using_extv): validate_subreg

> > > > >         before call gen_lowpart.

> > > > > ---

> > > > >  gcc/expmed.c | 6 +++++-

> > > > >  1 file changed, 5 insertions(+), 1 deletion(-)

> > > > >

> > > > > diff --git a/gcc/expmed.c b/gcc/expmed.c

> > > > > index 3143f38e057..10d62d857a8 100644

> > > > > --- a/gcc/expmed.c

> > > > > +++ b/gcc/expmed.c

> > > > > @@ -1571,12 +1571,16 @@ extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,

> > > > >

> > > > >    if (GET_MODE (target) != ext_mode)

> > > > >      {

> > > > > +      machine_mode tmode = GET_MODE (target);

> > > > >        /* Don't use LHS paradoxical subreg if explicit truncation is needed

> > > > >          between the mode of the extraction (word_mode) and the target

> > > > >          mode.  Instead, create a temporary and use convert_move to set

> > > > >          the target.  */

> > > > >        if (REG_P (target)

> > > > > -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> > > > > +         && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)

> > > > > +         && validate_subreg (ext_mode, tmode,

> > > > > +                             target,

> > > > > +                             subreg_lowpart_offset (ext_mode, tmode)))

> > > > >         {

> > > > >           target = gen_lowpart (ext_mode, target);

> > > >

> > > > That would be equivalent to use gen_lowpart_if_possible?

> > > No, target will be changed to NULL_RTX.

> > > But it does avoid ICE since maybe_expand_insn can legitimate operands,

> > > but I doubt it will introduce other bugs since the target has been

> > > changed here.

> > >

> > > I think the validate_subreg solution is plain and straightforward,

> > > just like it's done in

> > > r11-7515-g0ad6de3883a1641f7ec0bd9cf56d41fa5b313dae.

> >

> > That guards an explicit gen_rtx_SUBREG, here we're using gen_lowpart.

> > It's not an obvious match to validate gen_lowpart with validate_subreg,

> > I thought that gen_lowpart_if_possible would be prefered.  You obviously

> > have to adjust the code, like

> >

> >   rtx tem;

> >   if (...

> >       && (tem = gen_lowpart_if_possible (ext_mode, target))

> >     {

> Yes, update patch

>

>   bootstrapped and regtested on x86_64-linux-gnu{-m32,}.

>   Ok for trunk?


OK.

Thanks,
Richard.

> gcc/ChangeLog:

>

>         * expmed.c (extract_bit_field_using_extv): Use

>         gen_lowpart_if_possible instead of gen_lowpart to avoid ICE.

> ---

>  gcc/expmed.c | 6 ++++--

>  1 file changed, 4 insertions(+), 2 deletions(-)

>

> diff --git a/gcc/expmed.c b/gcc/expmed.c

> index 3143f38e057..59734d4841c 100644

> --- a/gcc/expmed.c

> +++ b/gcc/expmed.c

> @@ -1571,14 +1571,16 @@ extract_bit_field_using_extv (const

> extraction_insn *extv, rtx op0,

>

>    if (GET_MODE (target) != ext_mode)

>      {

> +      rtx temp;

>        /* Don't use LHS paradoxical subreg if explicit truncation is needed

>          between the mode of the extraction (word_mode) and the target

>          mode.  Instead, create a temporary and use convert_move to set

>          the target.  */

>        if (REG_P (target)

> -         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))

> +         && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode)

> +         && (temp = gen_lowpart_if_possible (ext_mode, target)))

>         {

> -         target = gen_lowpart (ext_mode, target);

> +         target = temp;

>           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

>             spec_target_subreg = target;

>         }

> --

> 2.27.0

>

> >        target = tem;

> > ...

> >

> > Richard.

> >

> > >

> > > >

> > > > >           if (partial_subreg_p (GET_MODE (spec_target), ext_mode))

> > > > > --

> > > > > 2.27.0

> > > > >

> > >

> > >

> > >

> > > --

> > > BR,

> > > Hongtao

>

>

>

> --

> BR,

> Hongtao

Patch

diff --git a/gcc/expmed.c b/gcc/expmed.c
index 3143f38e057..10d62d857a8 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -1571,12 +1571,16 @@  extract_bit_field_using_extv (const extraction_insn *extv, rtx op0,
 
   if (GET_MODE (target) != ext_mode)
     {
+      machine_mode tmode = GET_MODE (target);
       /* Don't use LHS paradoxical subreg if explicit truncation is needed
 	 between the mode of the extraction (word_mode) and the target
 	 mode.  Instead, create a temporary and use convert_move to set
 	 the target.  */
       if (REG_P (target)
-	  && TRULY_NOOP_TRUNCATION_MODES_P (GET_MODE (target), ext_mode))
+	  && TRULY_NOOP_TRUNCATION_MODES_P (tmode, ext_mode)
+	  && validate_subreg (ext_mode, tmode,
+			      target,
+			      subreg_lowpart_offset (ext_mode, tmode)))
 	{
 	  target = gen_lowpart (ext_mode, target);
 	  if (partial_subreg_p (GET_MODE (spec_target), ext_mode))