On Wed, Sep 8, 2021 at 1:10 PM Segher Boessenkool
<seg...@kernel.crashing.org> wrote:
>
> Hi!
>
> On Wed, Sep 08, 2021 at 08:42:44AM +0200, Richard Biener wrote:
> > On Wed, Sep 8, 2021 at 1:08 AM Segher Boessenkool
> > <seg...@kernel.crashing.org> wrote:
> > > The core of the problem is that subreg of pseudos has three meanings:
> > > -- Paradoxical subregs;
> > > -- Actual subregs;
> > > -- "bit_cast" thingies: treat the same bits as something else. Like
> > > looking at the bits of a float as its memory image.
> > >
> > > Ignoring paradoxical subregs (as well as subregs of mem, which should
> > > have disappeared by now), and subregs of hard registers as well (those
> > > have *different* semantics after all), the other two kinds can be mixed,
> > > and *have to* be mixed, because subregs of subregs are non-canonical.
> > >
> > > Is there any reason why not to allow this kind of subreg?
> >
> > In fact the causing rev. in question
> > (d2874d905647a1d146dafa60199d440e837adc4d)
> > made all those subregs "valid" in terms of what validate_subreg is verifying
> > and thus now the few places using validate_subreg to check whether some
> > subreg is valid will now happily do float<->int converting subregs.
>
> Like in PR102154:
>
> error: unrecognizable insn:
> (insn 11 10 12 2 (set (reg:SF 118 [ <retval> ])
> (subreg:SF (reg:DI 122) 0))
>
> This generated valid code before, and now it is not anymore. If the
> patch was meant to allow *more*, it failed that goal: it allows *less*.
>
> The issue is that we do not allow SF subregs in all cases. We do not
> store SFmode quantities in IEEE SP format in registers normally, so it
> would require expensive extra code to allow such subregs.
>
> Whatever pass made these subregs is wrong, because they do not pass
> recog().
>
> > I do agree that those subregs should be allowed and that the above rev. is
> > a strict improvement (given it removes a lot of "but allow special case X
> > because target Y wants it" cases by simply allowing all of them). But the
> > previous code seems to have papered over quite some backend issues.
>
> It is not a good idea to do allow all those things. Most backends can
> only support a few combinations of them, and everything else results in
> *worse* machine code, best case, and more and more complicated (and more
> buggy!) backend code.
>
> But that is a code quality issue. The current problem is that we have
> at least PR102211 and PR102154 (as well as reports elsewhere of bugs on
> other targets). Code that used before doesn't anymore, and we have no
> clear way out, no recommendation how to fix this and a) keep the same
> functionality without huge changes, and b) keep the same machine code
> quality.
>
> I do not think something like that can be done. That is why I am asking
> for the patch to be reverted until all of the groundwork for it has been
> done. That includes making generic testcases that show how such subregs
> behave, so that we can see in testresults what changes do to existing
> targets.
Richi,
As was mentioned in an earlier reply, the patch removed comments in
the code itself that explained why the code was necessary:
/* ??? 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. */
/* ??? Similarly, e.g. with (subreg:DF (reg:TI)). Though store_bit_field
is the culprit here, and not the backends. */
The sources of the problem are issues in backends and issues in the
common part of the compiler. Removal of this functionality should
start with a preemptive analysis of the compiler to determine how this
ugly behavior is being utilized and a plan to fix all backends and the
common code generation paths in the compiler, not a sweeping change to
the backend that flushes out problems.
The GCC Regression policy places the burden on the developer whose
patch introduced the problem. Liuhong did not test any other targets
although the code comments clearly stated that the behavior was
required by many targets, and he is not stepping forward to resolve
the problems. This patch should be reverted until the problems
exposed by it can be addressed in a careful and deliberate manner.
Thanks, David
