On Tue, Aug 31, 2021 at 8:48 AM Hongtao Liu <[email protected]> wrote:
>
> On Tue, Aug 31, 2021 at 2:30 PM Hongtao Liu <[email protected]> wrote:
> >
> > On Tue, Aug 31, 2021 at 2:11 PM Richard Biener
> > <[email protected]> wrote:
> > >
> > > On Fri, Aug 27, 2021 at 6:50 AM Hongtao Liu <[email protected]> wrote:
> > > >
> > > > On Thu, Aug 26, 2021 at 7:09 PM Richard Biener via Gcc-patches
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Thu, Aug 26, 2021 at 12:50 PM Richard Sandiford
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Richard Biener via Gcc-patches <[email protected]> writes:
> > > > > > > On Thu, Aug 26, 2021 at 11:06 AM Richard Sandiford
> > > > > > > <[email protected]> wrote:
> > > > > > >>
> > > > > > >> Richard Biener via Gcc-patches <[email protected]> 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.
> let me test the patch which removed the upper code.
Yes, that's what I was refering to.
Richard.
> > > > >
> > > > > 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.
> > >
> > > Hmm, so looks like you pushed the variant massaging extract_bit_field.
> > > Above
> > > we supported to instead "fix" validate_subreg to allow the HFmode subreg.
> > >
> > > So maybe we should revert and try that?
> > This one:
> >
> > > + /* ???Similarly like (subreg:DI (reg:SF), also allow (subreg:SI
> > > (reg:HF))
> > > + here. Though extract_bit_field is the culprit here, not the
> > > backends. */
> > > + else if (known_gt (regsize, osize) && known_gt (osize, isize)
> > > + && FLOAT_MODE_P (imode) && INTEGRAL_MODE_P (omode))
> > > + ;
> >
> > or this one
> >
> > + machine_mode tmode = GET_MODE (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)
> > + /* When validate_subreg doesn't allow subreg between integer mode
> > + and float mode with different size, It will hit gcc_assert in
> > + gen_lowpart_general. Also subreg like (subreg:DI (reg:SF)) is
> > + not really needed, codes like below will be finally generated.
> > + (set (reg:SI 1)
> > + (and:SI (reg:DI 2) -1))
> > + (set (reg:SF 3)
> > + (subreg:SF (reg:SI 1))) */
> > + && FLOAT_MODE_P (tmode) && INTEGRAL_MODE_P (mode)
> > + && maybe_ne (GET_MODE_SIZE (tmode), GET_MODE_SIZE (mode)))
> > {
> > target = gen_lowpart (ext_mode, target);
> > if (partial_subreg_p (GET_MODE (spec_target), ext_mode))
> >
> > or the proposed patch in PR102133 which may risk falling down a rabbit hole?
> >
> > gcc_checking_assert (!x
> > || !(TREE_CODE (t) == SSA_NAME || is_gimple_reg (t))
> > || (use_register_for_decl (t)
> > - ? (REG_P (x)
> > + ? (REG_P (x) || SUBREG_P (x)
> > || (GET_CODE (x) == CONCAT
> > && (REG_P (XEXP (x, 0))
> > || SUBREG_P (XEXP (x, 0)))
> >
> >
> > >
> > > Richard.
> > >
> > > > --
> > > > BR,
> > > > Hongtao
> >
> >
> >
> > --
> > BR,
> > Hongtao
>
>
>
> --
> BR,
> Hongtao
