Re: [PATCH] Fix SFmode subreg of DImode and TImode

[Segher Boessenkool <segher@kernel.crashing.org>]

On Wed, Sep 08, 2021 at 08:39:31PM +0200, Richard Biener wrote:
> On September 8, 2021 7:08:09 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> >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.
> 
> Heh, I understood your earlier reply that you supported the change in principle based on the fact that nested subregs are invalid.

Ah.  No.  I meant to lament the fact that we use subregs for multiple
things, so that doing a bit_cast of a real subreg has to be expressed as
just one subreg, which is clearly sub-optimal for most backends.

I say *has to* because a subreg of a subreg is not valid RTL; it has to
be written as just one subreg.  Which makes thing more confusing and
confused than this already non-trivial thing has to be.

> Now, I don't think that validate_subreg is supposed to be the decision maker on what a target allows.

Right, some target hook or macro or whatnot should.

> For subregs of hardregs we seem to have a good way of validating, but
> what do we have for subregs of pseudos? Is it the passes generating
> the new unsupported subregs that should do different things? Should
> validate_subreg use a target hook to allow those special casings we
> removed which all were necessary just for specific targets but
> appearantly did not do any harm for other targets? 

Currently, we can disallow things in predicates and/or the instruction
conditions.

> Can you give advice as to how to address the needs of the HFmode subregs on x86 if not by adding another (generic) narrow exception in validate_subreg? 

If I knew what the problem was, perhaps?  Is this explained in some mail
somewhere?

> That said, I fail to see a good way forward after now two appearantly failed attempts. 
> 
> Btw, I'm fine reverting the patch but then what's the solution here? 

I think we should (longer term) get rid of the overloaded meanings and
uses of subregs.  One fairly simple thing is to make a new rtx code
"bit_cast" (or is there a nice short more traditional name for it?)

But that is not the core problem we had here.  The behaviour of the
generic parts of the compiler was changed, without testing if that
works on other targets but x86.  That is an understandable mistake, it
takes some experience to know where the morasses are.  But this change
should have been accompanied by testcases exercising the changed code.
We would have clearly seen there are issues then, simply by watching
gcc-testresults@ (and/or maintainers are on top of the test results
anyway).  Also, if there were testcases for this, we could have some
confidence that a change in this area is robust.


Segher


p.s. Very unrelated...  Should we have __builtin_bit_cast for C as well?
Is there any reason this could not work?