| 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. | expand |
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 >
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 > >
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
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 >> > > > >
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
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 > >> > > > > > > > >
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 > >> > > > > > > > >
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
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
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
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
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
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
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. */
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
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
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 >
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
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 >
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
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
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
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
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
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))