Re: [PATCH] avr: cc0 to mode_cc conversion

[Paul Koning via Gcc-patches]

> On Jan 5, 2021, at 8:54 AM, Senthil Kumar Selvaraj via Gcc-patches 
>  wrote:
> 
> 
> Senthil Kumar Selvaraj writes:
> 
>> Georg-Johann Lay writes:
>> 
>> ...
>>> 
>>> 2) We just saw 100reds of insns being dublicated, basically the whole
>>> machine description except for the few insns that leave cc alone.
>>> Isn't is possible to use define subst for the bulk of the insns and
>>> get a neat code that's better to grasp and to maintain?
>>> After all it's just appending a clobber of reg_cc, and in the current
>>> proposal almost 50% of the backend is just redundent repetitions of
>>> previous insns.
> 
> I could not find a way to get define_subst to do define_insn_and_split -
> other targets using the same approach (pdp11, h8300) have the
> duplication as well.

I ran into the same issue, I tried as well for the obvious reason.  I'm pretty 
sure someone told me (a) that doesn't work, and (b) the reason is xyzzy.  But I 
no long remember what the reason is, or even if I was told one.

The impression I have is that define_subst isn't a macro facility, even though 
it looks a bit like one, and that may be why it can't do what you want to do 
here.

paul

Re: [PATCH] avr: cc0 to mode_cc conversion

[Senthil Kumar Selvaraj via Gcc-patches]

Senthil Kumar Selvaraj writes:

> Georg-Johann Lay writes:
>
>>
>> Finally, some general remarks:
>
> The work on my github branch was not complete - I'd blindly followed
> whatever the CC0 Transition wiki mentioned (the first three steps of
> case #2), and fixed any regression fallout (for ATmega128).
>
> I intend to try out a define_subst/early clobber of reg_cc based
> approach (inspired by the cris port) and see if that can help avoid the
> proliferation of define_insn_and_splits. Will update how that works out.

I had some time this past week to try implementing some of the changes
you suggested.

>
>>
>> 2) We just saw 100reds of insns being dublicated, basically the whole
>> machine description except for the few insns that leave cc alone.
>> Isn't is possible to use define subst for the bulk of the insns and
>> get a neat code that's better to grasp and to maintain?
>> After all it's just appending a clobber of reg_cc, and in the current
>> proposal almost 50% of the backend is just redundent repetitions of
>> previous insns.

I could not find a way to get define_subst to do define_insn_and_split -
other targets using the same approach (pdp11, h8300) have the
duplication as well.

>>
>> 4) Many insns don't have reloads and don't need to be turned into a
>> splitter + yet another insns, it should be all right to clobber
>> reg_cc from the very start.  Or am I missing something?  I think
>> I marked all places, but it should be easy enough to spot them.

If I remove the define_insn_and_split and add a (clobber (reg:CC
REG_CC)) to the define_insn itself for xcall patterns, then the producer
of the pattern (define_expand, output template of
define-insn-and-split/define-split etc.. or C code) needs to modified to
include the clobber of REG_CC in a PARALLEL, so that's a whole bunch of
changes.

If that is done at define_expand, for example, then similar patterns
that do not use the hard regs (non-call variants) will also need to be
modified to add the clobber, and therefore there's no point in
define_insn without clobber and split after reload with clobber for
those patterns.

Did I get that right?

FWIW, I'm also working on a parallel implementation that clobbers REG_CC
in all patterns from the start (with matching clobbers in define_expand
etc..) - still not in good enough shape though. It will avoid
duplication, but at the expense of modification of nearly every pattern
to emit or accept a clobber of REG_CC.

Regards
Senthil

Re: [PATCH] avr: cc0 to mode_cc conversion

[abebeos via Gcc-patches]

On Fri, 18 Dec 2020 at 20:31, Segher Boessenkool 
wrote:

> On Fri, Dec 18, 2020 at 06:13:22PM +0100, Georg-Johann Lay wrote:
> > Segher Boessenkool schrieb:
> > >On Thu, Dec 17, 2020 at 10:07:22AM -0500, Paul Koning wrote:
> > >>>On Dec 17, 2020, at 6:21 AM, Segher Boessenkool
> > >>> wrote:
> > >>>On Thu, Dec 17, 2020 at 02:15:51PM +0530, Senthil Kumar Selvaraj via
> > >>>Gcc-patches wrote:
> > The work on my github branch was not complete - I'd blindly followed
> > whatever the CC0 Transition wiki mentioned (the first three steps of
> > case #2), and fixed any regression fallout (for ATmega128).
>
[...]

@Senthil Kumar Selvaraj 

Remember that a full polish of the avr-backend is beyond the scope of the
task/bounty:

avr-cc0 ---[eliminate cc0]---> avr-non-cc0 with similar (or even worse)
quality.

Possibly this one is relevant as for "effort":

https://gcc.gnu.org/pipermail/gcc/2020-April/000455.html
as mentioned here:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92729#c9

Re: [PATCH] avr: cc0 to mode_cc conversion

[Segher Boessenkool]

On Fri, Dec 18, 2020 at 06:13:22PM +0100, Georg-Johann Lay wrote:
> Segher Boessenkool schrieb:
> >On Thu, Dec 17, 2020 at 10:07:22AM -0500, Paul Koning wrote:
> >>>On Dec 17, 2020, at 6:21 AM, Segher Boessenkool 
> >>> wrote:
> >>>On Thu, Dec 17, 2020 at 02:15:51PM +0530, Senthil Kumar Selvaraj via 
> >>>Gcc-patches wrote:
> The work on my github branch was not complete - I'd blindly followed
> whatever the CC0 Transition wiki mentioned (the first three steps of
> case #2), and fixed any regression fallout (for ATmega128).
> 
> I intend to try out a define_subst/early clobber of reg_cc based
> approach (inspired by the cris port) and see if that can help avoid the
> proliferation of define_insn_and_splits. Will update how that works out.
> >>>Yeah, case #2 does not necessarily give the best result, but it usually
> >>>is the least work to do, so certainly a good choice under time pressure.
> >>I was under the impression from what I read in the blog a year or two ago 
> >>(when I did the pdp11 ccmode work) that "case 2" is the better answer for 
> >>machines in which pretty much every operation touches the condition 
> >>codes.  In other words, I understood that case 1 would for such machines 
> >>not be the right answer -- it wasn't just that "case 2 is easier".
> >>
> >>Did I misunderstand?  Is there a reason for machines such as pdp11, in 
> >>which basically every operation that handles data, even a move 
> >>instruction, touches CC, to use approach 1?
> >
> >No, you didn't misunderstand.  I said "not necessarily" for a reason :-)
> >
> >If there are move insns that do *not* clobber CC, it can be different,
> >but if even move instructions do, a case #2 conversion is a good choice.
> >
> >(This is all my opinion, but I think it is not controversial.)
> 
> As far as I understand, targets that clobber condition code in mov
> or add3 must use approach #2.

If there is no way to reload without clobbering CC, yeah.  Often you
*can* reload that way, at big cost (push the flags to the stack around
the move, for example), but if you need that for common insns, this is
no good.

> #1 is not a problem if the target
> still uses IRA (like avr does), but LRA cannot handle clobbers of the
> condition code (in terms or explicit clobbers).

(still uses reload)

I'm not sure what the LRA thing means?  The rs6000 port has many
patterns that clobber some specific CR reg.  If it is important here
that there is only one such reg, thyen we also clobber the CA reg in
many patterns (the carry bit, there is only one of those).

Or is this only wrt mov and add as well?

> Hence, targets that clobber cc in mov or add3 must
> use #2 because IRA/reload will also be kicked out before very soon.
> 
> Or am I missing something?

compare-elim.c claims this same thing:
  There is a set of targets whose general-purpose move or addition
  instructions clobber the flags.  These targets cannot split their
  CBRANCH/CSTORE etc patterns before reload is complete, lest reload
  itself insert these instructions in between the flags setter and user.
so you might well be right :-)

Thanks,


Segher

Re: [PATCH] avr: cc0 to mode_cc conversion

[Georg-Johann Lay via Gcc-patches]

Segher Boessenkool schrieb:

On Thu, Dec 17, 2020 at 10:07:22AM -0500, Paul Koning wrote:

On Dec 17, 2020, at 6:21 AM, Segher Boessenkool  
wrote:
On Thu, Dec 17, 2020 at 02:15:51PM +0530, Senthil Kumar Selvaraj via 
Gcc-patches wrote:

The work on my github branch was not complete - I'd blindly followed
whatever the CC0 Transition wiki mentioned (the first three steps of
case #2), and fixed any regression fallout (for ATmega128).

I intend to try out a define_subst/early clobber of reg_cc based
approach (inspired by the cris port) and see if that can help avoid the
proliferation of define_insn_and_splits. Will update how that works out.

Yeah, case #2 does not necessarily give the best result, but it usually
is the least work to do, so certainly a good choice under time pressure.

I was under the impression from what I read in the blog a year or two ago (when I did the pdp11 
ccmode work) that "case 2" is the better answer for machines in which pretty much every 
operation touches the condition codes.  In other words, I understood that case 1 would for such 
machines not be the right answer -- it wasn't just that "case 2 is easier".

Did I misunderstand?  Is there a reason for machines such as pdp11, in which 
basically every operation that handles data, even a move instruction, touches 
CC, to use approach 1?


No, you didn't misunderstand.  I said "not necessarily" for a reason :-)

If there are move insns that do *not* clobber CC, it can be different,
but if even move instructions do, a case #2 conversion is a good choice.

(This is all my opinion, but I think it is not controversial.)


As far as I understand, targets that clobber condition code in mov
or add3 must use approach #2.  #1 is not a problem if the target
still uses IRA (like avr does), but LRA cannot handle clobbers of the
condition code (in terms or explicit clobbers).

Hence, targets that clobber cc in mov or add3 must
use #2 because IRA/reload will also be kicked out before very soon.

Or am I missing something?

Johann

Re: [PATCH] avr: cc0 to mode_cc conversion

[Segher Boessenkool]

On Thu, Dec 17, 2020 at 10:07:22AM -0500, Paul Koning wrote:
> > On Dec 17, 2020, at 6:21 AM, Segher Boessenkool 
> >  wrote:
> > On Thu, Dec 17, 2020 at 02:15:51PM +0530, Senthil Kumar Selvaraj via 
> > Gcc-patches wrote:
> >> The work on my github branch was not complete - I'd blindly followed
> >> whatever the CC0 Transition wiki mentioned (the first three steps of
> >> case #2), and fixed any regression fallout (for ATmega128).
> >> 
> >> I intend to try out a define_subst/early clobber of reg_cc based
> >> approach (inspired by the cris port) and see if that can help avoid the
> >> proliferation of define_insn_and_splits. Will update how that works out.
> > 
> > Yeah, case #2 does not necessarily give the best result, but it usually
> > is the least work to do, so certainly a good choice under time pressure.
> 
> I was under the impression from what I read in the blog a year or two ago 
> (when I did the pdp11 ccmode work) that "case 2" is the better answer for 
> machines in which pretty much every operation touches the condition codes.  
> In other words, I understood that case 1 would for such machines not be the 
> right answer -- it wasn't just that "case 2 is easier".
> 
> Did I misunderstand?  Is there a reason for machines such as pdp11, in which 
> basically every operation that handles data, even a move instruction, touches 
> CC, to use approach 1?

No, you didn't misunderstand.  I said "not necessarily" for a reason :-)

If there are move insns that do *not* clobber CC, it can be different,
but if even move instructions do, a case #2 conversion is a good choice.

(This is all my opinion, but I think it is not controversial.)


Segher

Re: [PATCH] avr: cc0 to mode_cc conversion

[Paul Koning via Gcc-patches]

> On Dec 17, 2020, at 6:21 AM, Segher Boessenkool  
> wrote:
> 
> Hi!
> 
> On Thu, Dec 17, 2020 at 02:15:51PM +0530, Senthil Kumar Selvaraj via 
> Gcc-patches wrote:
>> The work on my github branch was not complete - I'd blindly followed
>> whatever the CC0 Transition wiki mentioned (the first three steps of
>> case #2), and fixed any regression fallout (for ATmega128).
>> 
>> I intend to try out a define_subst/early clobber of reg_cc based
>> approach (inspired by the cris port) and see if that can help avoid the
>> proliferation of define_insn_and_splits. Will update how that works out.
> 
> Yeah, case #2 does not necessarily give the best result, but it usually
> is the least work to do, so certainly a good choice under time pressure.

I was under the impression from what I read in the blog a year or two ago (when 
I did the pdp11 ccmode work) that "case 2" is the better answer for machines in 
which pretty much every operation touches the condition codes.  In other words, 
I understood that case 1 would for such machines not be the right answer -- it 
wasn't just that "case 2 is easier".

Did I misunderstand?  Is there a reason for machines such as pdp11, in which 
basically every operation that handles data, even a move instruction, touches 
CC, to use approach 1?

paul

Re: [PATCH] avr: cc0 to mode_cc conversion

[Segher Boessenkool]

Hi!

On Thu, Dec 17, 2020 at 02:15:51PM +0530, Senthil Kumar Selvaraj via 
Gcc-patches wrote:
> The work on my github branch was not complete - I'd blindly followed
> whatever the CC0 Transition wiki mentioned (the first three steps of
> case #2), and fixed any regression fallout (for ATmega128).
> 
> I intend to try out a define_subst/early clobber of reg_cc based
> approach (inspired by the cris port) and see if that can help avoid the
> proliferation of define_insn_and_splits. Will update how that works out.

Yeah, case #2 does not necessarily give the best result, but it usually
is the least work to do, so certainly a good choice under time pressure.

> > 1) Most inline asm will now clobber cc.  Is this handled automatically,
> > or why is there no addition to target asm clobbers? There is already
> > code out there that clobbers "cc", hence that should still work / be
> > recognized.
> 
> The wiki suggests using TARGET_MD_ASM_ADJUST hook to do that, that is
> not yet done.

This is really easy to do.  Look at examples from other ports (i386 does
its flags register as well I believe).  This is pretty important to get
right, so it would be good if you could include it.

> > 4) Many insns don't have reloads and don't need to be turned into a
> > splitter + yet another insns, it should be all right to clobber
> > reg_cc from the very start.  Or am I missing something?  I think
> > I marked all places, but it should be easy enough to spot them.

That works if you only ever generate it at instruction stream locations
where it is not yet live.  We do this for the CA (carry) register (bit)
on powerpc: this works because we generate new insns using it during
expand only.

> > 5) Combine now sees cbranch instead of compare as outer insn, and
> > hence will query respective costs.  I'd expect to see some extensions
> > to cost hooks to adjust for that?
> >
> > You can have a look at the combine dumps to see whether respective
> > patterns are still generated, or if they are rejected.  (No test case
> > will tell you that, you must know what you are doing).

Use -dp to inspect the costs in the generated asm file ("c=X", it also
shows the name of the MD pattern used), and -fdump-rtl-combine-all to
have the combine dump say a lot about costs as well (just use -dap if
you don't mind the over a hundred debug files that generates).

> > 7) Attribute cc is no more used.  It should be removed.

Well, is there code "in the wild" using it still?

> AFAICT, modes other than CCmode are required only when adding support for
> setting specific condition code bits.

When some instruction sets the condition mode bits in a different way /
with a different meaning than normal comparison instructions do, more
generally.


Segher

Re: [PATCH] avr: cc0 to mode_cc conversion

[abebeos via Gcc-patches]

On Sun, 13 Dec 2020 at 20:51, Georg-Johann Lay  wrote:

> > (I really tried to follow this
> https://gcc.gnu.org/contribute.html#patches,
> > but my stomach)
> >
> > Hi there all!
> >
> > The attached patch contains a new avr-backend, stripped from cc0.
> >
> > The author is gcc maintainer Snethil Kumar Selvaraj (saaadhu),
>
> Hi, AFAIK Senthil has write-after-approval state.  The only avr
> maintainer, at least according to MAINTAINERS, is still Denis, cf.
>
>
> http://gcc.gnu.org/git/?p=gcc.git;a=blob;f=MAINTAINERS;h=32f8a2b72923b791f9687d6a2d555a1780535078;hb=HEAD#l59
>
> I allowed me CCing them.
>
> > the source can be found here:
> >
> > https://github.com/saaadhu/gcc-avr-cc0/tree/avr-cc0-squashed
> >
> > The gcc/g++ testsuites show zero regressions, tested with:
> >
> > https://github.com/abebeos/avr-gnu
> >
> > and confirmed with another testsetup, see:
> >
> > https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561489.html
> >
> > Some more background information within:
> >
> > https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561668.html
> >
> > and
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92729
> >
> > (i have this dark feeling that I did the patch submission wrong...
> pressing
> > Send anyways!)
>

(hey, "between us": where did my real-name came from in your email? I
choosed to appear here with an alias "abebeos" - not that it's difficult to
find my real name, but still, it's my choice...)


>
> Sometimes it's more convenient to have a .diff text file, but I don't
> see anything wrong with your submission.


ok

And what it really nice to
> have hunks defined for "(define_" or "^(define_") in your git diff
> setup.  That way it's easier to track in which entity of the machine
> description a change is located.  Just like when you have a change
> in the middle of a long C function, and the diff chunk spells out
> the C function in which it is located.
>

ok

> diff --git a/gcc/config/avr/avr-dimode.md b/gcc/config/avr/avr-dimode.md
>
[...] - (review)

Thank you for taking the time to review.

The author (currently not available) assessed the work as "semi working":

The (lets say) integrator (me), assessed the work as "good enough" - given
status of the gcc10 avr-backend this should be the prioriity (saving it
from extinction).

Possibly all details should be ignored for now, thus it can be merged.

My process/decision (given circumstances) is to "not touch even whitespace"
in the 0-regression-patch.

So, what remains for me now is to produce a tiny document for the
bounty-backers, and just hope that the folks here finds peace to do
the right thing.

Over

a
n
d

Out!

.