On Thu, Aug 26, 2021 at 7:09 PM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Thu, Aug 26, 2021 at 12:50 PM Richard Sandiford
> <richard.sandif...@arm.com> wrote:
> >
> > Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > > On Thu, Aug 26, 2021 at 11:06 AM Richard Sandiford
> > > <richard.sandif...@arm.com> wrote:
> > >>
> > >> Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > >> > One thought I had is whether we can "fix" validate_subreg to have less
> > >> > "weird" allowed float-int
> > >> > special cases.  As said upthread I think that we either should allow
> > >> > all of those, implying that
> > >> > subregs work semantically as if there's subregs to same-sized integer
> > >> > modes inbetween or
> > >> > disallow them all and make sure we're actually doing that explicitely.
> > >> >
> > >> > For example
> > >> >
> > >> >   /* ??? 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))
> > >> >     ;
> > >> >
> > >> > I can't decipther rtl.text as to what the semantics of such a subreg is
> > >> > given the docs hand-wave about WORDS_BIG_ENDIAN vs.
> > >> > FLOAT_WORDS_BIG_ENDIAN but don't actually say what happens
> > >> > when you mix those in a subreg.  So maybe the above should
> > >> > have explicitely have WORDS_BIG_ENDIAN == FLOAT_WORDS_BIG_ENDIAN.
> > >> >
> > >> > But then the world would be much simpler if subregs of non-same size
> > >> > modes have explicit documentation for the mode kinds we have.
> > >>
> > >> Yeah.  Although validate_subreg was a good idea, some of the mode checks
> > >> are IMO a failed experiment.  The hope was that eventually we'd remove
> > >> all those special exceptions once the culprit has been fixed.  However,
> > >> the code is over 16 years old at this point and those changes never
> > >> happened.
> > >>
> > >> Nested subregs aren't a thing (thankfully) and one of the big 
> > >> disadvantages
> > >> of the current validate_subreg mode-changing rules is that they aren't
> > >> transitive.  This can artificially require temporary pseudos for things
> > >> that could be expressed directly as a single subreg.
> > >
> > > And that's what the proposed patch does (add same-mode size integer mode
> > > punning intermediate subregs).
> > >
> > > So if that's not supposed to be necessary then why restrict subregs at 
> > > all?
> >
> > I was trying to say: I'm not sure we should.
> >
> > > I mean you seem to imply that the semantics would be clear and 
> > > well-defined
> > > (to you - not to me).  The only thing is that of course not all subregs 
> > > are
> > > "implemented" by a target (or can be, w/o spilling).
> >
> > Yeah.  That's for TARGET_CAN_CHANGE_MODE_CLASS to decide.
> > But it only comes in to play during RA or when trying to take
> > the subreg of a particular hard register.  Transitivity doesn't
> > matter so much for the hard register case since the result of
> > simplify_gen_subreg should then be another hard register.
> >
> > > Which means - we should adjust validate_subreg with another special-case
> > > or rather generalize the existing ones to an overall set that makes more
> > > sense?
> >
> > Maybe it's too radical, but I would whether we should just get rid of:
> >
> >   /* ??? 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;
> >     }
> >
> > altogether.
>
> Yeah, I would fully support this.  Maybe replace it with a comment
> but I don't know what it should say.
>
> Richard.
>
> >
> > Thanks,
> > Richard

I'm going to upstream the patch.

-- 
BR,
Hongtao

Reply via email to