Re: Deprecating cc0 (and consequently cc0 targets)

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Oct 29, 2019 at 8:34 PM Jeff Law <law@redhat.com> wrote:
>
> On 10/29/19 6:26 AM, John Paul Adrian Glaubitz wrote:
> > Hello!
> >
> > We have raised $5000 to support anyone willing to work on this for the
> > m68k target [1]. We really need the m68k to stay as it's essential to
> > be able to compile for Linux/m68k, NetBSD/m68k and AROS/m68k (a new
> > and ABI-compatible AmigaOS clone). I'm sure other communities like in the
> > NeXTStep, Atari and Mac/m68k forums are interested in keeping the backend
> > as well. I'm confident we will be able to raise even more money as the
> > community is large and active.
> >
> >> If someone is interested in possibly doing a conversion, I would
> >> strongly recommend reviewing the best documentation we have on the
> >> subject:
> >>
> >> https://gcc.gnu.org/wiki/CC0Transition
> >>
> >> I worked with my son about a year ago to transition the v850 to MODE_CC
> >> using that page as a guideline.  We also have a partial transition for
> >> the H8/300 port (not far enough to even build anything yet).  I had
> >> hoped we would finish the H8/300 port last year and would have tackled
> >> another, but events have gotten in the way.
> >
> > Any chance that the unfinished code can be shared? I'm looking for any
> > blueprints that can be used as a guidance for the work on the m68k
> > backend.
> I'm not sure it'd be all that useful.  The v850 bits would be a better
> starting point.  The partial H8 transition isn't committed anywhere
> because it would certainly break the port.
>
> >
> > I have also created a guide on how to set up a QEMU virtual machine running
> > a fully fledged Debian/m68k which can be used for the development work [2].
> >
> >> The transition isn't terribly hard, but does take time because of the
> >> number of patterns that have to be changed.
> >
> > I have already looked at the conversion done on the V850 backend [3] but so far
> > I don't understand enough of the register representation stuff to be able
> > to know where to start. I'm seeing that all the "cc" attributes are being removed
> > but not replaced?
> Right. The cc attributes were used to describe which condition codes
> were valid after each insn.  That information now has to be carried by
> the mode of the condition code register.
>
> I'll walk you through one example from the H8 port.  This isn't complete
> enough to even test that it builds.
>
>
> > (define_insn "*addsi_h8300"
> >   [(set (match_operand:SI 0 "register_operand" "=r,r")
> >         (plus:SI (match_operand:SI 1 "register_operand" "%0,0")
> >                  (match_operand:SI 2 "h8300_src_operand" "n,r")))]
> >   "TARGET_H8300"
> > {
> >   return output_plussi (operands);
> > }
> >   [(set (attr "length")
> >         (symbol_ref "compute_plussi_length (operands)"))
> >    (set (attr "cc")
> >         (symbol_ref "compute_plussi_cc (operands)"))])
>
>
> That needs to turn into a define_insn_and_split.  The idea is the form
> above is what's seen prior to register allocation and reloading.  After
> reloading we turn it into a form with an attached clobber.  We achieve
> this by rewriting the pattern into a define_insn_and_split like this:
>
> > (define_insn_and_split "*addsi_h8300"
> >    [(set (match_operand:SI 0 "register_operand" "=r,r")
> >         (plus:SI (match_operand:SI 1 "register_operand" "%0,0")
> >                  (match_operand:SI 2 "h8300_src_operand" "n,r")))]
> >   "TARGET_H8300"
> >   "#"
> >   "reload_completed"
> >   [(parallel [(set (match_dup 0) (plus:SI (match_dup 1) (match_dup 2)))
> >               (clobber (reg:CC CC_REG))])])
>
> We added the "#" which indicates the pattern has to be split and will
> never generate assembly code in and of itself.  The "reload_completed"
> indicates when the spliter should fire (after reload).  The subsequent
> RTL is how the pattern should be rewritten after reload (adds the clobber).
>
> Then you need a new pattern which matches the output of the splitter
> with the attached clobber.  Something like this:
>
> > (define_insn "*addsi_h8300_clobber_flags"
> >   [(set (match_operand:SI 0 "register_operand" "=r,r")
> >        (plus:SI (match_operand:SI 1 "register_operand" "%0,0")
> >                 (match_operand:SI 2 "h8300_src_operand" "n,r")))
> >    (clobber (reg:CC CC_REG))]
> >   "TARGET_H8300"
> >  {
> >    return output_plussi (operands);
> >  }
> This is the form that we'll use after reload.  It explicitly clobbers
> the condition codes which is the key here.

I think the wiki has better examples.  That said, I wonder how much can
be automated here, for example when just considering CCmode (m68k has
setcc IIRC) then for example all define_insns like

(define_insn "*cmpsi_cf"
  [(set (cc0)
        (compare (match_operand:SI 0 "nonimmediate_operand" "mrKs,r")
                 (match_operand:SI 1 "general_operand" "r,mrKs")))]
  "TARGET_COLDFIRE"
{...}

could be rewritten based on seeing (set (cc0) ...) to a (set (reg:CC
CC_REG) ...)
plus adding a "&& reload_completed" condition.  That could be done
transparently by the .md file reader even?

The various expanders need to be handled manually I guess, though somehow
automagically generating the define_insn_and_split might be possible
as well, no?

That is, the work is quite repetitive (looking at m68k.md) and for this reason
error-prone.  But a case#2 conversion could essentially be automated?

That is, do you see it as impossible to implement a case#2 cc0 "conversion"
in gen*?

> Some have claimed this might be easier with define_subst, but I wandered
> it a bit and it didn't seem necessarily simpler in the end.  But maybe
> someone with more experience with define_subst could make the transition
> simpler.
>
> Anyway, that has to be done for each and every pattern that modifies the
> condition codes.  It's a ton of mechanical work.  The m68k has the
> additional complication that condition codes can be either in cc0 or
> fcc0.  So that has to be taken into account as well.

I don't see that it does this currently though (cc0 doesn't allow this).

> You also have to rewrite the tst/cmp and conditional branching patterns
> to use the new style.
>
> Sadly until you do *all* of that you don't have anything you can really
> test.  And even once you've done all that, the code quality is going to
> suffer because we're not doing any compare/tst elimination.  To enable
> that you have to go back and start adding patterns like this:

For m68k what helps is that you could concentrate on TARGET_COLDFIRE
(or !TARGET_COLDFIRE).  Maybe even split m68k.md along this.

A lot of the define_insns also look like optimization (so could be disabled
for the moment).

> (define_insn "*addsi_h8300_set_flags"
>   [(set (reg:CCNZ CC_REGNUM)
>          (compare CCNZ (plus:SI (match_operand:SI 1 "register_operand"
> "%0,0")
>                                 (match_operand:SI 2 "register_operand"
> "n, r"))))
>     (set (match_operand:SI 0 "register_operand" "=r,r")
>          (plus:SI (match_dup 1) (match_dup 2)))]
>   "reload_completed && TARGET_H8300"
>  {
>    return output_plussi (operands);
>  }
>
> This is where we describe (via the mode of the condition code register)
> what condition codes are set and how to extract them.  These patterns
> can be added incrementally.  But again, it's going to be a ton of work.
>
> My son is definitely still interested.  But I doubt he'd likely be able
> to take up the task until the spring/summer of 2020.  Additionally,
> there's some question of whether or not him doing the work could be
> viewed as a conflict of interest WRT the bug bounty because of his
> relationship to me.
>
>
> jeff
>