* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-08 19:16 ` Segher Boessenkool
@ 2021-09-09 1:23 ` Hongtao Liu
2021-09-09 1:31 ` Hongtao Liu
2021-09-09 6:16 ` Richard Biener
2 siblings, 0 replies; 24+ messages in thread
From: Hongtao Liu @ 2021-09-09 1:23 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Richard Biener, Michael Meissner, Peter Bergner, Bill Schmidt,
GCC Patches, David Edelsohn
On Thu, Sep 9, 2021 at 3:17 AM Segher Boessenkool
<segher@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 <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.
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-08 19:16 ` Segher Boessenkool
2021-09-09 1:23 ` Hongtao Liu
@ 2021-09-09 1:31 ` Hongtao Liu
2021-09-09 6:16 ` Richard Biener
2 siblings, 0 replies; 24+ messages in thread
From: Hongtao Liu @ 2021-09-09 1:31 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Richard Biener, Michael Meissner, Peter Bergner, Bill Schmidt,
GCC Patches, David Edelsohn
On Thu, Sep 9, 2021 at 3:17 AM Segher Boessenkool
<segher@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 <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?
it's at [1], target_hook may not be the best solution, if other
targets support HFmode, they may encounter the same problem, and
revisit it again.
[1] https://gcc.gnu.org/pipermail/gcc-patches/2021-August/576827.html
>
> > 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-08 19:16 ` Segher Boessenkool
2021-09-09 1:23 ` Hongtao Liu
2021-09-09 1:31 ` Hongtao Liu
@ 2021-09-09 6:16 ` Richard Biener
2021-09-09 22:53 ` Michael Meissner
2021-09-09 23:48 ` Segher Boessenkool
2 siblings, 2 replies; 24+ messages in thread
From: Richard Biener @ 2021-09-09 6:16 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt,
Peter Bergner, Will Schmidt
On Wed, Sep 8, 2021 at 9:18 PM Segher Boessenkool
<segher@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 <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 subreg _is_ bit_cast. What is odd to me is that a "disallowed" subreg
like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
Of course that's nested and invalid but just push the inner subreg to a
new pseudo and the thing becomes valid.
That was my original idea for dealing with the issue described in
the thread including
https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578437.html
where the original issue is that extract_integral_bit_field creates
a subreg that's not valid according to validate_subreg. So I thought
well, extracting bitfields should be done on integer modes (thus
pun everything to ints and back), but that had even more(?) fallout.
> 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.
Well, that only works if some maintainers that are familiar enough
with all this chime in ;) It's stage1 so it's understandable that some
people (like me ...) are tyring to help people making progress even
if that involves trying to decipher 30 years of GCC history in this
area (without much success in the end as we see) ;)
Given validate_subreg's _intent_ is to disallow float subregs
that change size exceptions like
/* ??? 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))
;
look arbitrary and odd.
Richard.
>
> Segher
>
>
> p.s. Very unrelated... Should we have __builtin_bit_cast for C as well?
> Is there any reason this could not work?
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-09 6:16 ` Richard Biener
@ 2021-09-09 22:53 ` Michael Meissner
2021-09-09 23:48 ` Segher Boessenkool
1 sibling, 0 replies; 24+ messages in thread
From: Michael Meissner @ 2021-09-09 22:53 UTC (permalink / raw)
To: Richard Biener
Cc: Segher Boessenkool, Michael Meissner, GCC Patches,
David Edelsohn, Bill Schmidt, Peter Bergner, Will Schmidt
On Thu, Sep 09, 2021 at 08:16:16AM +0200, Richard Biener wrote:
> But subreg _is_ bit_cast. What is odd to me is that a "disallowed" subreg
> like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
> validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
> Of course that's nested and invalid but just push the inner subreg to a
> new pseudo and the thing becomes valid.
Basically I only checked the mode of the SUBREG and the value in SUBREG_REG. I
agree (subreg:SF (subreg:SI (reg:TI))) should not be allowed.
When I wrote the changes we didn't see SUBREG's as much to access the bottom or
top element of a structure that was held in a larger int field. If the patch
is not reverted, then we have to deal with this somehow. Such as with my patch
that allows (subreg:SF (reg:TI xxx)) and (subreg:SF (reg:DI xxx)).
The target hook idea might be too big of a hammer. I would need to play with
it to see if there are other losses.
On the PowerPC, the problematical subregs only occur when you are moving a
register from GPR to a vector/float register or vice versa. If you move a
subreg between two GPR registers or two vector/float registers, then you can
just do the copy.
Part of the issue is for some moves, you need to use a temporary register to
convert the results, but of course we have the long standing 'all moves for a
mode must be done in a single insn that covers all of the cases'. Then you get
into secondary reload territory.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-09 6:16 ` Richard Biener
2021-09-09 22:53 ` Michael Meissner
@ 2021-09-09 23:48 ` Segher Boessenkool
2021-09-10 3:09 ` Hongtao Liu
2021-09-10 10:53 ` Richard Biener
1 sibling, 2 replies; 24+ messages in thread
From: Segher Boessenkool @ 2021-09-09 23:48 UTC (permalink / raw)
To: Richard Biener
Cc: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt,
Peter Bergner, Will Schmidt
On Thu, Sep 09, 2021 at 08:16:16AM +0200, Richard Biener wrote:
> > 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 subreg _is_ bit_cast.
It is not. (subreg:M (reg:N) O) for O>0, little-endian, is not a
bit_cast. It is taking a part of a register, or a single register from
a multi-register thing. Paradoxicals are not bit-casts either.
Subregs from or to (but not both) integer modes are generally bit_cast,
yeah.
> What is odd to me is that a "disallowed" subreg
> like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
> validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
> Of course that's nested and invalid but just push the inner subreg to a
> new pseudo and the thing becomes valid.
Bingo.
And many targets have strange rules for bit-strings in which modes can
be used as bit-strings in which other modes, and at what offsets in
which registers. Now perhaps none of that is optimal (I bet it isn't),
but changing this without a transition plan simply does not work.
> > 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.
>
> Well, that only works if some maintainers that are familiar enough
> with all this chime in ;)
Not really. It works always. And it works way better than the
pandemonium we now have with broken targets left and right.
With testcases anyone can see if any specific target is broken here.
> It's stage1 so it's understandable that some
> people (like me ...) are tyring to help people making progress even
> if that involves trying to decipher 30 years of GCC history in this
> area (without much success in the end as we see) ;)
Yeah :-) And my thanks to you and everyone involved for tackling this
problematic part of GCC, which has been neglected and patched over for
way too long. But from that same history it follows that anything you
do not super carefully (with testing everywhere) will cause some serious
problems. And nonse of these are easy to fix at all -- there is a
*reason* targets did this nastiness.
> > p.s. Very unrelated... Should we have __builtin_bit_cast for C as well?
> > Is there any reason this could not work?
Still interested in this btw :-) (And still very unrelated.)
Segher
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-09 23:48 ` Segher Boessenkool
@ 2021-09-10 3:09 ` Hongtao Liu
2021-09-10 10:54 ` Richard Biener
` (2 more replies)
2021-09-10 10:53 ` Richard Biener
1 sibling, 3 replies; 24+ messages in thread
From: Hongtao Liu @ 2021-09-10 3:09 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Richard Biener, Michael Meissner, Peter Bergner, Bill Schmidt,
GCC Patches, David Edelsohn
On Fri, Sep 10, 2021 at 7:49 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Thu, Sep 09, 2021 at 08:16:16AM +0200, Richard Biener wrote:
> > > 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 subreg _is_ bit_cast.
>
> It is not. (subreg:M (reg:N) O) for O>0, little-endian, is not a
> bit_cast. It is taking a part of a register, or a single register from
> a multi-register thing. Paradoxicals are not bit-casts either.
>
> Subregs from or to (but not both) integer modes are generally bit_cast,
> yeah.
>
> > What is odd to me is that a "disallowed" subreg
> > like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
> > validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
> > Of course that's nested and invalid but just push the inner subreg to a
> > new pseudo and the thing becomes valid.
>
> Bingo.
>
> And many targets have strange rules for bit-strings in which modes can
> be used as bit-strings in which other modes, and at what offsets in
> which registers. Now perhaps none of that is optimal (I bet it isn't),
> but changing this without a transition plan simply does not work.
>
> > > 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.
> >
> > Well, that only works if some maintainers that are familiar enough
> > with all this chime in ;)
>
> Not really. It works always. And it works way better than the
> pandemonium we now have with broken targets left and right.
>
> With testcases anyone can see if any specific target is broken here.
>
> > It's stage1 so it's understandable that some
> > people (like me ...) are tyring to help people making progress even
> > if that involves trying to decipher 30 years of GCC history in this
> > area (without much success in the end as we see) ;)
>
> Yeah :-) And my thanks to you and everyone involved for tackling this
> problematic part of GCC, which has been neglected and patched over for
> way too long. But from that same history it follows that anything you
> do not super carefully (with testing everywhere) will cause some serious
Frankly, testing everywhere is too heavy a burden for developers,
after all, everyone has a limited variety of machines, and may not be
familiar with using other targets' simulators.
And back to the problem we were trying to solve at the beginning
(subreg:HF(reg:SI)), I guess this is not just a problem in x86
backend, any backend can encounter similar problems, that's why we
remove all the weird cases in validate_subreg.
> problems. And nonse of these are easy to fix at all -- there is a
> *reason* targets did this nastiness.
>
> > > p.s. Very unrelated... Should we have __builtin_bit_cast for C as well?
> > > Is there any reason this could not work?
>
> Still interested in this btw :-) (And still very unrelated.)
>
>
> Segher
--
BR,
Hongtao
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-10 3:09 ` Hongtao Liu
@ 2021-09-10 10:54 ` Richard Biener
2021-09-10 11:25 ` Hongtao Liu
2021-09-10 14:08 ` David Edelsohn
2021-09-10 15:32 ` Segher Boessenkool
2 siblings, 1 reply; 24+ messages in thread
From: Richard Biener @ 2021-09-10 10:54 UTC (permalink / raw)
To: Hongtao Liu
Cc: Segher Boessenkool, Michael Meissner, Peter Bergner,
Bill Schmidt, GCC Patches, David Edelsohn
On Fri, Sep 10, 2021 at 5:03 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Fri, Sep 10, 2021 at 7:49 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > On Thu, Sep 09, 2021 at 08:16:16AM +0200, Richard Biener wrote:
> > > > 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 subreg _is_ bit_cast.
> >
> > It is not. (subreg:M (reg:N) O) for O>0, little-endian, is not a
> > bit_cast. It is taking a part of a register, or a single register from
> > a multi-register thing. Paradoxicals are not bit-casts either.
> >
> > Subregs from or to (but not both) integer modes are generally bit_cast,
> > yeah.
> >
> > > What is odd to me is that a "disallowed" subreg
> > > like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
> > > validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
> > > Of course that's nested and invalid but just push the inner subreg to a
> > > new pseudo and the thing becomes valid.
> >
> > Bingo.
> >
> > And many targets have strange rules for bit-strings in which modes can
> > be used as bit-strings in which other modes, and at what offsets in
> > which registers. Now perhaps none of that is optimal (I bet it isn't),
> > but changing this without a transition plan simply does not work.
> >
> > > > 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.
> > >
> > > Well, that only works if some maintainers that are familiar enough
> > > with all this chime in ;)
> >
> > Not really. It works always. And it works way better than the
> > pandemonium we now have with broken targets left and right.
> >
> > With testcases anyone can see if any specific target is broken here.
> >
> > > It's stage1 so it's understandable that some
> > > people (like me ...) are tyring to help people making progress even
> > > if that involves trying to decipher 30 years of GCC history in this
> > > area (without much success in the end as we see) ;)
> >
> > Yeah :-) And my thanks to you and everyone involved for tackling this
> > problematic part of GCC, which has been neglected and patched over for
> > way too long. But from that same history it follows that anything you
> > do not super carefully (with testing everywhere) will cause some serious
> Frankly, testing everywhere is too heavy a burden for developers,
> after all, everyone has a limited variety of machines, and may not be
> familiar with using other targets' simulators.
> And back to the problem we were trying to solve at the beginning
> (subreg:HF(reg:SI)), I guess this is not just a problem in x86
> backend, any backend can encounter similar problems, that's why we
> remove all the weird cases in validate_subreg.
So can you please revert the change for now? I think we need to go
back to the issue in extract_bit_field - does it somehow work to use
validate_subreg to avoid creating the subreg we ICE on in the first
place and what happens then to code quality?
Thanks,
Richard.
> > problems. And nonse of these are easy to fix at all -- there is a
> > *reason* targets did this nastiness.
> >
> > > > p.s. Very unrelated... Should we have __builtin_bit_cast for C as well?
> > > > Is there any reason this could not work?
> >
> > Still interested in this btw :-) (And still very unrelated.)
> >
> >
> > Segher
>
>
>
> --
> BR,
> Hongtao
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-10 10:54 ` Richard Biener
@ 2021-09-10 11:25 ` Hongtao Liu
2021-09-10 12:34 ` Hongtao Liu
0 siblings, 1 reply; 24+ messages in thread
From: Hongtao Liu @ 2021-09-10 11:25 UTC (permalink / raw)
To: Richard Biener
Cc: Segher Boessenkool, Michael Meissner, Peter Bergner,
Bill Schmidt, GCC Patches, David Edelsohn
On Fri, Sep 10, 2021 at 6:54 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Fri, Sep 10, 2021 at 5:03 AM Hongtao Liu <crazylht@gmail.com> wrote:
> >
> > On Fri, Sep 10, 2021 at 7:49 AM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > >
> > > On Thu, Sep 09, 2021 at 08:16:16AM +0200, Richard Biener wrote:
> > > > > 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 subreg _is_ bit_cast.
> > >
> > > It is not. (subreg:M (reg:N) O) for O>0, little-endian, is not a
> > > bit_cast. It is taking a part of a register, or a single register from
> > > a multi-register thing. Paradoxicals are not bit-casts either.
> > >
> > > Subregs from or to (but not both) integer modes are generally bit_cast,
> > > yeah.
> > >
> > > > What is odd to me is that a "disallowed" subreg
> > > > like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
> > > > validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
> > > > Of course that's nested and invalid but just push the inner subreg to a
> > > > new pseudo and the thing becomes valid.
> > >
> > > Bingo.
> > >
> > > And many targets have strange rules for bit-strings in which modes can
> > > be used as bit-strings in which other modes, and at what offsets in
> > > which registers. Now perhaps none of that is optimal (I bet it isn't),
> > > but changing this without a transition plan simply does not work.
> > >
> > > > > 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.
> > > >
> > > > Well, that only works if some maintainers that are familiar enough
> > > > with all this chime in ;)
> > >
> > > Not really. It works always. And it works way better than the
> > > pandemonium we now have with broken targets left and right.
> > >
> > > With testcases anyone can see if any specific target is broken here.
> > >
> > > > It's stage1 so it's understandable that some
> > > > people (like me ...) are tyring to help people making progress even
> > > > if that involves trying to decipher 30 years of GCC history in this
> > > > area (without much success in the end as we see) ;)
> > >
> > > Yeah :-) And my thanks to you and everyone involved for tackling this
> > > problematic part of GCC, which has been neglected and patched over for
> > > way too long. But from that same history it follows that anything you
> > > do not super carefully (with testing everywhere) will cause some serious
> > Frankly, testing everywhere is too heavy a burden for developers,
> > after all, everyone has a limited variety of machines, and may not be
> > familiar with using other targets' simulators.
> > And back to the problem we were trying to solve at the beginning
> > (subreg:HF(reg:SI)), I guess this is not just a problem in x86
> > backend, any backend can encounter similar problems, that's why we
> > remove all the weird cases in validate_subreg.
>
> So can you please revert the change for now? I think we need to go
> back to the issue in extract_bit_field - does it somehow work to use
> validate_subreg to avoid creating the subreg we ICE on in the first
> place and what happens then to code quality?
Sure, let me test the patch.
>
> Thanks,
> Richard.
>
> > > problems. And nonse of these are easy to fix at all -- there is a
> > > *reason* targets did this nastiness.
> > >
> > > > > p.s. Very unrelated... Should we have __builtin_bit_cast for C as well?
> > > > > Is there any reason this could not work?
> > >
> > > Still interested in this btw :-) (And still very unrelated.)
> > >
> > >
> > > Segher
> >
> >
> >
> > --
> > BR,
> > Hongtao
--
BR,
Hongtao
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-10 11:25 ` Hongtao Liu
@ 2021-09-10 12:34 ` Hongtao Liu
0 siblings, 0 replies; 24+ messages in thread
From: Hongtao Liu @ 2021-09-10 12:34 UTC (permalink / raw)
To: Richard Biener
Cc: Segher Boessenkool, Michael Meissner, Peter Bergner,
Bill Schmidt, GCC Patches, David Edelsohn
On Fri, Sep 10, 2021 at 7:25 PM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Fri, Sep 10, 2021 at 6:54 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Fri, Sep 10, 2021 at 5:03 AM Hongtao Liu <crazylht@gmail.com> wrote:
> > >
> > > On Fri, Sep 10, 2021 at 7:49 AM Segher Boessenkool
> > > <segher@kernel.crashing.org> wrote:
> > > >
> > > > On Thu, Sep 09, 2021 at 08:16:16AM +0200, Richard Biener wrote:
> > > > > > 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 subreg _is_ bit_cast.
> > > >
> > > > It is not. (subreg:M (reg:N) O) for O>0, little-endian, is not a
> > > > bit_cast. It is taking a part of a register, or a single register from
> > > > a multi-register thing. Paradoxicals are not bit-casts either.
> > > >
> > > > Subregs from or to (but not both) integer modes are generally bit_cast,
> > > > yeah.
> > > >
> > > > > What is odd to me is that a "disallowed" subreg
> > > > > like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
> > > > > validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
> > > > > Of course that's nested and invalid but just push the inner subreg to a
> > > > > new pseudo and the thing becomes valid.
> > > >
> > > > Bingo.
> > > >
> > > > And many targets have strange rules for bit-strings in which modes can
> > > > be used as bit-strings in which other modes, and at what offsets in
> > > > which registers. Now perhaps none of that is optimal (I bet it isn't),
> > > > but changing this without a transition plan simply does not work.
> > > >
> > > > > > 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.
> > > > >
> > > > > Well, that only works if some maintainers that are familiar enough
> > > > > with all this chime in ;)
> > > >
> > > > Not really. It works always. And it works way better than the
> > > > pandemonium we now have with broken targets left and right.
> > > >
> > > > With testcases anyone can see if any specific target is broken here.
> > > >
> > > > > It's stage1 so it's understandable that some
> > > > > people (like me ...) are tyring to help people making progress even
> > > > > if that involves trying to decipher 30 years of GCC history in this
> > > > > area (without much success in the end as we see) ;)
> > > >
> > > > Yeah :-) And my thanks to you and everyone involved for tackling this
> > > > problematic part of GCC, which has been neglected and patched over for
> > > > way too long. But from that same history it follows that anything you
> > > > do not super carefully (with testing everywhere) will cause some serious
> > > Frankly, testing everywhere is too heavy a burden for developers,
> > > after all, everyone has a limited variety of machines, and may not be
> > > familiar with using other targets' simulators.
> > > And back to the problem we were trying to solve at the beginning
> > > (subreg:HF(reg:SI)), I guess this is not just a problem in x86
> > > backend, any backend can encounter similar problems, that's why we
> > > remove all the weird cases in validate_subreg.
> >
> > So can you please revert the change for now? I think we need to go
> > back to the issue in extract_bit_field - does it somehow work to use
> > validate_subreg to avoid creating the subreg we ICE on in the first
> > place and what happens then to code quality?
> Sure, let me test the patch.
Survive regtest, code quality seems to be ok.
But lose some performance.
.cfi_startproc
- pinsrw $0, %edi, %xmm0
+ movw %di, -2(%rsp)
+ pinsrw $0, -2(%rsp), %xmm0
ret
.cfi_endproc
Anyway I'll post the patches first and see if I can fix the
performance issue in the x86 backend.
> >
> > Thanks,
> > Richard.
> >
> > > > problems. And nonse of these are easy to fix at all -- there is a
> > > > *reason* targets did this nastiness.
> > > >
> > > > > > p.s. Very unrelated... Should we have __builtin_bit_cast for C as well?
> > > > > > Is there any reason this could not work?
> > > >
> > > > Still interested in this btw :-) (And still very unrelated.)
> > > >
> > > >
> > > > Segher
> > >
> > >
> > >
> > > --
> > > BR,
> > > Hongtao
>
>
>
> --
> BR,
> Hongtao
--
BR,
Hongtao
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-10 3:09 ` Hongtao Liu
2021-09-10 10:54 ` Richard Biener
@ 2021-09-10 14:08 ` David Edelsohn
2021-09-10 14:14 ` Hongtao Liu
2021-09-10 15:32 ` Segher Boessenkool
2 siblings, 1 reply; 24+ messages in thread
From: David Edelsohn @ 2021-09-10 14:08 UTC (permalink / raw)
To: Hongtao Liu
Cc: Segher Boessenkool, Richard Biener, Michael Meissner,
Peter Bergner, Bill Schmidt, GCC Patches
On Thu, Sep 9, 2021 at 11:03 PM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Fri, Sep 10, 2021 at 7:49 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > On Thu, Sep 09, 2021 at 08:16:16AM +0200, Richard Biener wrote:
> > > > 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 subreg _is_ bit_cast.
> >
> > It is not. (subreg:M (reg:N) O) for O>0, little-endian, is not a
> > bit_cast. It is taking a part of a register, or a single register from
> > a multi-register thing. Paradoxicals are not bit-casts either.
> >
> > Subregs from or to (but not both) integer modes are generally bit_cast,
> > yeah.
> >
> > > What is odd to me is that a "disallowed" subreg
> > > like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
> > > validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
> > > Of course that's nested and invalid but just push the inner subreg to a
> > > new pseudo and the thing becomes valid.
> >
> > Bingo.
> >
> > And many targets have strange rules for bit-strings in which modes can
> > be used as bit-strings in which other modes, and at what offsets in
> > which registers. Now perhaps none of that is optimal (I bet it isn't),
> > but changing this without a transition plan simply does not work.
> >
> > > > 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.
> > >
> > > Well, that only works if some maintainers that are familiar enough
> > > with all this chime in ;)
> >
> > Not really. It works always. And it works way better than the
> > pandemonium we now have with broken targets left and right.
> >
> > With testcases anyone can see if any specific target is broken here.
> >
> > > It's stage1 so it's understandable that some
> > > people (like me ...) are tyring to help people making progress even
> > > if that involves trying to decipher 30 years of GCC history in this
> > > area (without much success in the end as we see) ;)
> >
> > Yeah :-) And my thanks to you and everyone involved for tackling this
> > problematic part of GCC, which has been neglected and patched over for
> > way too long. But from that same history it follows that anything you
> > do not super carefully (with testing everywhere) will cause some serious
> Frankly, testing everywhere is too heavy a burden for developers,
> after all, everyone has a limited variety of machines, and may not be
> familiar with using other targets' simulators.
Hongtao,
This is why the GNU Toolchain community sponsors the GCC Compile Farm.
Not every architecture is provided, but a good subset. And there are
a few, powerful Linux on Power systems in the cluster, such as gcc135.
And the comments in the code warned of port-specific problems. Even
if you cannot test the patch yourself, you should have publicly
requested that other port maintainers test the patch to shake out
problems.
The Tree-SSA passes are mostly target-independent, but RTL is
target-dependent, which can elicit widely different behavior in the
RTL passes. When you make changes to RTL passes in the common parts
of the compiler, you must consider the impact on all targets. GCC
explicitly supports a wide variety of architectures, ABIs and OSes.
All of the developers strive to ensure that changes don't adversely
affect any targets.
Thanks, David
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-10 14:08 ` David Edelsohn
@ 2021-09-10 14:14 ` Hongtao Liu
0 siblings, 0 replies; 24+ messages in thread
From: Hongtao Liu @ 2021-09-10 14:14 UTC (permalink / raw)
To: David Edelsohn
Cc: Segher Boessenkool, Richard Biener, Michael Meissner,
Peter Bergner, Bill Schmidt, GCC Patches
On Fri, Sep 10, 2021 at 10:08 PM David Edelsohn <dje.gcc@gmail.com> wrote:
>
> On Thu, Sep 9, 2021 at 11:03 PM Hongtao Liu <crazylht@gmail.com> wrote:
> >
> > On Fri, Sep 10, 2021 at 7:49 AM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > >
> > > On Thu, Sep 09, 2021 at 08:16:16AM +0200, Richard Biener wrote:
> > > > > 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 subreg _is_ bit_cast.
> > >
> > > It is not. (subreg:M (reg:N) O) for O>0, little-endian, is not a
> > > bit_cast. It is taking a part of a register, or a single register from
> > > a multi-register thing. Paradoxicals are not bit-casts either.
> > >
> > > Subregs from or to (but not both) integer modes are generally bit_cast,
> > > yeah.
> > >
> > > > What is odd to me is that a "disallowed" subreg
> > > > like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
> > > > validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
> > > > Of course that's nested and invalid but just push the inner subreg to a
> > > > new pseudo and the thing becomes valid.
> > >
> > > Bingo.
> > >
> > > And many targets have strange rules for bit-strings in which modes can
> > > be used as bit-strings in which other modes, and at what offsets in
> > > which registers. Now perhaps none of that is optimal (I bet it isn't),
> > > but changing this without a transition plan simply does not work.
> > >
> > > > > 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.
> > > >
> > > > Well, that only works if some maintainers that are familiar enough
> > > > with all this chime in ;)
> > >
> > > Not really. It works always. And it works way better than the
> > > pandemonium we now have with broken targets left and right.
> > >
> > > With testcases anyone can see if any specific target is broken here.
> > >
> > > > It's stage1 so it's understandable that some
> > > > people (like me ...) are tyring to help people making progress even
> > > > if that involves trying to decipher 30 years of GCC history in this
> > > > area (without much success in the end as we see) ;)
> > >
> > > Yeah :-) And my thanks to you and everyone involved for tackling this
> > > problematic part of GCC, which has been neglected and patched over for
> > > way too long. But from that same history it follows that anything you
> > > do not super carefully (with testing everywhere) will cause some serious
> > Frankly, testing everywhere is too heavy a burden for developers,
> > after all, everyone has a limited variety of machines, and may not be
> > familiar with using other targets' simulators.
>
> Hongtao,
>
> This is why the GNU Toolchain community sponsors the GCC Compile Farm.
> Not every architecture is provided, but a good subset. And there are
> a few, powerful Linux on Power systems in the cluster, such as gcc135.
>
I see.
> And the comments in the code warned of port-specific problems. Even
> if you cannot test the patch yourself, you should have publicly
> requested that other port maintainers test the patch to shake out
> problems.
Yes, sorry for the inconvenience.
>
> The Tree-SSA passes are mostly target-independent, but RTL is
> target-dependent, which can elicit widely different behavior in the
> RTL passes. When you make changes to RTL passes in the common parts
> of the compiler, you must consider the impact on all targets. GCC
> explicitly supports a wide variety of architectures, ABIs and OSes.
> All of the developers strive to ensure that changes don't adversely
> affect any targets.
>
> Thanks, David
--
BR,
Hongtao
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-10 3:09 ` Hongtao Liu
2021-09-10 10:54 ` Richard Biener
2021-09-10 14:08 ` David Edelsohn
@ 2021-09-10 15:32 ` Segher Boessenkool
2 siblings, 0 replies; 24+ messages in thread
From: Segher Boessenkool @ 2021-09-10 15:32 UTC (permalink / raw)
To: Hongtao Liu
Cc: Richard Biener, Michael Meissner, Peter Bergner, Bill Schmidt,
GCC Patches, David Edelsohn
On Fri, Sep 10, 2021 at 11:09:31AM +0800, Hongtao Liu wrote:
> On Fri, Sep 10, 2021 at 7:49 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > way too long. But from that same history it follows that anything you
> > do not super carefully (with testing everywhere) will cause some serious
> Frankly, testing everywhere is too heavy a burden for developers,
> after all, everyone has a limited variety of machines, and may not be
> familiar with using other targets' simulators.
Your change should be tested on enough relevant targets that we can have
confidence it works properly. That does not necessarily mean you have
to test everywhere yourself (although that is greatly appreciated, makes
life easier for everyone, including yourself, and as David points out
the cfarm is a great help).
So you didn't realise your patch would wreak havoc on most other targets
than what you tested on. It happens, it helps if you can avoid it, but
you learn only from things that go wrong :-)
(The patch has still not been reverted btw. I'll do this later tonight
if you don't).
> And back to the problem we were trying to solve at the beginning
> (subreg:HF(reg:SI)), I guess this is not just a problem in x86
> backend, any backend can encounter similar problems, that's why we
> remove all the weird cases in validate_subreg.
Expand should simply expand to two statements: one doing the real
subreg, the other doing the bit_cast.
Segher
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-09 23:48 ` Segher Boessenkool
2021-09-10 3:09 ` Hongtao Liu
@ 2021-09-10 10:53 ` Richard Biener
2021-09-10 15:05 ` Segher Boessenkool
1 sibling, 1 reply; 24+ messages in thread
From: Richard Biener @ 2021-09-10 10:53 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt,
Peter Bergner, Will Schmidt
On Fri, Sep 10, 2021 at 1:50 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Thu, Sep 09, 2021 at 08:16:16AM +0200, Richard Biener wrote:
> > > 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 subreg _is_ bit_cast.
>
> It is not. (subreg:M (reg:N) O) for O>0, little-endian, is not a
> bit_cast. It is taking a part of a register, or a single register from
> a multi-register thing. Paradoxicals are not bit-casts either.
>
> Subregs from or to (but not both) integer modes are generally bit_cast,
> yeah.
>
> > What is odd to me is that a "disallowed" subreg
> > like (subreg:SF (reg:TI ..) 0) magically becomes valid (in terms of
> > validate_subreg) if you rewrite it as (subreg:SF (subreg:SI (reg:TI ..) 0) 0).
> > Of course that's nested and invalid but just push the inner subreg to a
> > new pseudo and the thing becomes valid.
>
> Bingo.
>
> And many targets have strange rules for bit-strings in which modes can
> be used as bit-strings in which other modes, and at what offsets in
> which registers. Now perhaps none of that is optimal (I bet it isn't),
> but changing this without a transition plan simply does not work.
But we _do_ already allow some of them :/ Like
/* ??? 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))
;
so for the special case where 'regsize' matches osize it would be
a bit-cast of a full register from int to float. But as written it also
allows (subreg:XF (reg:TI)) which will likely wreck havoc?
Similar for the omode == word_mode check which allows
(subreg:DI (reg:TF ..)). That is, the existing special-cases look
too broad to me - and they probably exist because when validate_subreg
rejects sth then we can't put it together later when expand split it
into two subregs and a pseudo ...
> > > 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.
> >
> > Well, that only works if some maintainers that are familiar enough
> > with all this chime in ;)
>
> Not really. It works always. And it works way better than the
> pandemonium we now have with broken targets left and right.
>
> With testcases anyone can see if any specific target is broken here.
>
> > It's stage1 so it's understandable that some
> > people (like me ...) are tyring to help people making progress even
> > if that involves trying to decipher 30 years of GCC history in this
> > area (without much success in the end as we see) ;)
>
> Yeah :-) And my thanks to you and everyone involved for tackling this
> problematic part of GCC, which has been neglected and patched over for
> way too long. But from that same history it follows that anything you
> do not super carefully (with testing everywhere) will cause some serious
> problems. And nonse of these are easy to fix at all -- there is a
> *reason* targets did this nastiness.
>
> > > p.s. Very unrelated... Should we have __builtin_bit_cast for C as well?
> > > Is there any reason this could not work?
>
> Still interested in this btw :-) (And still very unrelated.)
Sure, why not ...
Richard.
>
> Segher
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-10 10:53 ` Richard Biener
@ 2021-09-10 15:05 ` Segher Boessenkool
2021-09-13 9:03 ` Richard Biener
0 siblings, 1 reply; 24+ messages in thread
From: Segher Boessenkool @ 2021-09-10 15:05 UTC (permalink / raw)
To: Richard Biener
Cc: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt,
Peter Bergner, Will Schmidt
On Fri, Sep 10, 2021 at 12:53:37PM +0200, Richard Biener wrote:
> On Fri, Sep 10, 2021 at 1:50 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > And many targets have strange rules for bit-strings in which modes can
> > be used as bit-strings in which other modes, and at what offsets in
> > which registers. Now perhaps none of that is optimal (I bet it isn't),
> > but changing this without a transition plan simply does not work.
>
> But we _do_ already allow some of them :/ Like
Yes. And all of this is old and ingrained, and targets depend on the
status quo, so changing this will need more care and planning and
cooperation. It certainly is a worthwhile thing to improve, but it is
not a small project, and it requires a plan.
> /* ??? 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))
> ;
>
> so for the special case where 'regsize' matches osize it would be
> a bit-cast of a full register from int to float. But as written it also
> allows (subreg:XF (reg:TI)) which will likely wreck havoc?
That does not pass the isize >= osize test? Or maybe I don't know
what XFmode is well enough :-) Hey I can read, I have source, and it
is Friday...
Ah. So XF has different size on 32-bit and on 64-bit, but that doesn't
even matter here.
> Similar for the omode == word_mode check which allows
> (subreg:DI (reg:TF ..)). That is, the existing special-cases look
> too broad to me - and they probably exist because when validate_subreg
> rejects sth then we can't put it together later when expand split it
> into two subregs and a pseudo ...
I said it before, and I'll say it again, it is a very important point:
expand should not try to optimise this, *at all*. And not just this,
not *anything*. Expand's job in the current compiler is only to
translate Gimple to RTL, and nothing more.
When later passes try to optimise things they will always ask the target
if it agrees with the change (all RTL has to pass recog() for example).
This simplifies life.
Segher
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Fix SFmode subreg of DImode and TImode
2021-09-10 15:05 ` Segher Boessenkool
@ 2021-09-13 9:03 ` Richard Biener
0 siblings, 0 replies; 24+ messages in thread
From: Richard Biener @ 2021-09-13 9:03 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt,
Peter Bergner, Will Schmidt
On Fri, Sep 10, 2021 at 5:07 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Sep 10, 2021 at 12:53:37PM +0200, Richard Biener wrote:
> > On Fri, Sep 10, 2021 at 1:50 AM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > And many targets have strange rules for bit-strings in which modes can
> > > be used as bit-strings in which other modes, and at what offsets in
> > > which registers. Now perhaps none of that is optimal (I bet it isn't),
> > > but changing this without a transition plan simply does not work.
> >
> > But we _do_ already allow some of them :/ Like
>
> Yes. And all of this is old and ingrained, and targets depend on the
> status quo, so changing this will need more care and planning and
> cooperation. It certainly is a worthwhile thing to improve, but it is
> not a small project, and it requires a plan.
>
> > /* ??? 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))
> > ;
> >
> > so for the special case where 'regsize' matches osize it would be
> > a bit-cast of a full register from int to float. But as written it also
> > allows (subreg:XF (reg:TI)) which will likely wreck havoc?
>
> That does not pass the isize >= osize test? Or maybe I don't know
> what XFmode is well enough :-) Hey I can read, I have source, and it
> is Friday...
TImode is 16 bytes but XFmode is 12. I meant to construct a case
which passes all the tests but where definitely such kind of subreg
is very odd to be "allowed" by validate_subreg - a target may of course
have means to make sense of it (I don't see how x87 would though).
> Ah. So XF has different size on 32-bit and on 64-bit, but that doesn't
> even matter here.
>
> > Similar for the omode == word_mode check which allows
> > (subreg:DI (reg:TF ..)). That is, the existing special-cases look
> > too broad to me - and they probably exist because when validate_subreg
> > rejects sth then we can't put it together later when expand split it
> > into two subregs and a pseudo ...
>
> I said it before, and I'll say it again, it is a very important point:
> expand should not try to optimise this, *at all*. And not just this,
> not *anything*. Expand's job in the current compiler is only to
> translate Gimple to RTL, and nothing more.
+1 (or +10!), but unfortunately expand _is_ an important part
of optimization - in particular when it gets to avoiding stack usage
since we can never get rid of all effects of allocating a stack slot.
Splitting to multiple insns with pseudos should be fine though, but
it at least seems that splitting (subreg:FLOAT (reg:INT)) into
(subreg:INT (reg:INT)) (subreg:FLOAT (reg:INT)) isn't always valid.
Richard.
^ permalink raw reply [flat|nested] 24+ messages in thread