On Thu, Sep 9, 2021 at 3:17 AM Segher Boessenkool
<seg...@kernel.crashing.org> wrote:
>
> 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
> > <seg...@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.
Since there is no good solution yet, I think we can add a target hook
(targetm.validate_subreg) to recover the original removed code as a
default behavior, and then x86 itself defines the hook as empty
(meaning that the backend allows all kinds of subreg).
something like
@@ -922,6 +922,9 @@ validate_subreg (machine_mode omode, machine_mode imode,
poly_uint64 regsize = REGMODE_NATURAL_SIZE (imode);
+ if (!targetm.valiate_subreg (omode, imode, reg, offset))
+ return false;
+
/* Paradoxical subregs must have offset zero. */
if (maybe_gt (osize, isize))
return known_eq (offset, 0U);
>
> 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?
--
BR,
Hongtao
