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

[Hongtao Liu <crazylht@gmail.com>]

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