[PATCH 0/2] Revert r12-3277 since it caused regressions on many other targets.

[PATCH 0/2] Revert r12-3277 since it caused regressions on many other targets.

[liuhongt]

Hi:
  Details discussed in https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579170.html.
  
  Bootstrapped and regtested on x86_64-linux-gnu{-m32,}.
  Ok for trunk?

liuhongt (2):
  Revert "Get rid of all float-int special cases in validate_subreg."
  validate_subreg before call gen_lowpart to avoid ICE.

 gcc/emit-rtl.c | 40 ++++++++++++++++++++++++++++++++++++++++
 gcc/expmed.c   |  6 +++++-
 2 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH 1/2] Revert "Get rid of all float-int special cases in validate_subreg."

[liuhongt]

This reverts commit d2874d905647a1d146dafa60199d440e837adc4d.

PR target/102254
PR target/102154
PR target/102211
---
 gcc/emit-rtl.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 77ea8948ee8..ff3b4449b37 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -922,6 +922,46 @@ validate_subreg (machine_mode omode, machine_mode imode,
 
   poly_uint64 regsize = REGMODE_NATURAL_SIZE (imode);
 
+  /* ??? This should not be here.  Temporarily continue to allow word_mode
+     subregs of anything.  The most common offender is (subreg:SI (reg:DF)).
+     Generally, backends are doing something sketchy but it'll take time to
+     fix them all.  */
+  if (omode == word_mode)
+    ;
+  /* ??? Similarly, e.g. with (subreg:DF (reg:TI)).  Though store_bit_field
+     is the culprit here, and not the backends.  */
+  else if (known_ge (osize, regsize) && known_ge (isize, osize))
+    ;
+  /* Allow component subregs of complex and vector.  Though given the below
+     extraction rules, it's not always clear what that means.  */
+  else if ((COMPLEX_MODE_P (imode) || VECTOR_MODE_P (imode))
+	   && GET_MODE_INNER (imode) == omode)
+    ;
+  /* ??? x86 sse code makes heavy use of *paradoxical* vector subregs,
+     i.e. (subreg:V4SF (reg:SF) 0) or (subreg:V4SF (reg:V2SF) 0).  This
+     surely isn't the cleanest way to represent this.  It's questionable
+     if this ought to be represented at all -- why can't this all be hidden
+     in post-reload splitters that make arbitrarily mode changes to the
+     registers themselves.  */
+  else if (VECTOR_MODE_P (omode)
+	   && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
+    ;
+  /* Subregs involving floating point modes are not allowed to
+     change size.  Therefore (subreg:DI (reg:DF) 0) is fine, but
+     (subreg:SI (reg:DF) 0) isn't.  */
+  else if (FLOAT_MODE_P (imode) || FLOAT_MODE_P (omode))
+    {
+      if (! (known_eq (isize, osize)
+	     /* LRA can use subreg to store a floating point value in
+		an integer mode.  Although the floating point and the
+		integer modes need the same number of hard registers,
+		the size of floating point mode can be less than the
+		integer mode.  LRA also uses subregs for a register
+		should be used in different mode in on insn.  */
+	     || lra_in_progress))
+	return false;
+    }
+
   /* Paradoxical subregs must have offset zero.  */
   if (maybe_gt (osize, isize))
     return known_eq (offset, 0U);
-- 
2.27.0

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

[liuhongt]

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);
 	  if (partial_subreg_p (GET_MODE (spec_target), ext_mode))
-- 
2.27.0

Re: [PATCH 1/2] Revert "Get rid of all float-int special cases in validate_subreg."

[Richard Biener]

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

OK.

Richard.

> PR target/102254
> PR target/102154
> PR target/102211
> ---
>  gcc/emit-rtl.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index 77ea8948ee8..ff3b4449b37 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -922,6 +922,46 @@ validate_subreg (machine_mode omode, machine_mode imode,
>
>    poly_uint64 regsize = REGMODE_NATURAL_SIZE (imode);
>
> +  /* ??? This should not be here.  Temporarily continue to allow word_mode
> +     subregs of anything.  The most common offender is (subreg:SI (reg:DF)).
> +     Generally, backends are doing something sketchy but it'll take time to
> +     fix them all.  */
> +  if (omode == word_mode)
> +    ;
> +  /* ??? Similarly, e.g. with (subreg:DF (reg:TI)).  Though store_bit_field
> +     is the culprit here, and not the backends.  */
> +  else if (known_ge (osize, regsize) && known_ge (isize, osize))
> +    ;
> +  /* Allow component subregs of complex and vector.  Though given the below
> +     extraction rules, it's not always clear what that means.  */
> +  else if ((COMPLEX_MODE_P (imode) || VECTOR_MODE_P (imode))
> +          && GET_MODE_INNER (imode) == omode)
> +    ;
> +  /* ??? x86 sse code makes heavy use of *paradoxical* vector subregs,
> +     i.e. (subreg:V4SF (reg:SF) 0) or (subreg:V4SF (reg:V2SF) 0).  This
> +     surely isn't the cleanest way to represent this.  It's questionable
> +     if this ought to be represented at all -- why can't this all be hidden
> +     in post-reload splitters that make arbitrarily mode changes to the
> +     registers themselves.  */
> +  else if (VECTOR_MODE_P (omode)
> +          && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
> +    ;
> +  /* Subregs involving floating point modes are not allowed to
> +     change size.  Therefore (subreg:DI (reg:DF) 0) is fine, but
> +     (subreg:SI (reg:DF) 0) isn't.  */
> +  else if (FLOAT_MODE_P (imode) || FLOAT_MODE_P (omode))
> +    {
> +      if (! (known_eq (isize, osize)
> +            /* LRA can use subreg to store a floating point value in
> +               an integer mode.  Although the floating point and the
> +               integer modes need the same number of hard registers,
> +               the size of floating point mode can be less than the
> +               integer mode.  LRA also uses subregs for a register
> +               should be used in different mode in on insn.  */
> +            || lra_in_progress))
> +       return false;
> +    }
> +
>    /* Paradoxical subregs must have offset zero.  */
>    if (maybe_gt (osize, isize))
>      return known_eq (offset, 0U);
> --
> 2.27.0
>

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

[Richard Biener]

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
>

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

[Hongtao Liu]

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

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

[Segher Boessenkool]

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

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

[Richard Biener]

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
>> >
>
>
>

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

[Hongtao Liu]

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

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

[Hongtao Liu]

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

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

[Hongtao Liu]

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

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

[Richard Biener]

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

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

[Hongtao Liu]

[-- Attachment #1: Type: text/plain, Size: 4057 bytes --]

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

[-- Attachment #2: v2-0001-Check-modes_tieable_p-before-call-gen_lowpart-to-.patch --]
[-- Type: application/octet-stream, Size: 1069 bytes --]

From e1a63d41ad733e46f28cff540cfacc4742488782 Mon Sep 17 00:00:00 2001
From: liuhongt <hongtao.liu@intel.com>
Date: Fri, 10 Sep 2021 22:15:36 +0800
Subject: [PATCH v2] Check modes_tieable_p before call gen_lowpart to avoid ICE
 in validate_subreg.

gcc/ChangeLog:

	* expmed.c (extract_bit_field_using_extv): Check
	modes_tieable_p before call gen_lowpart.
---
 gcc/expmed.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/expmed.c b/gcc/expmed.c
index 3143f38e057..fc1c973033c 100644
--- a/gcc/expmed.c
+++ b/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))
-- 
2.27.0

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

[Segher Boessenkool]

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

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

[Richard Biener]

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

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

[Segher Boessenkool]

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

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

[Segher Boessenkool]

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

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

[Hongtao Liu]

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

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

[Hongtao Liu]

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

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

[Richard Biener]

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

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

[Hongtao Liu]

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

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

[Hongtao Liu]

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

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

[Richard Biener]

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
>

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

[Hongtao Liu]

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

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

[Richard Biener]

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

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

[Hongtao Liu]

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

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

[Tobias Burnus]

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

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

[Richard Biener]

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