Re: Deprecating cc0 (and consequently cc0 targets)

[Richard Sandiford <richard dot sandiford at arm dot com>]

Richard Biener <richard.guenther@gmail.com> writes:
> 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*?

I don't think we should do an automatic conversion in gen*,
since presumably that means that (cc0) sticks around in the .md files.
It would also make it harder to do follow-on work if what the .md file
says isn't what you actually get.

So yeah, some parts of the change might be automatable, but I think they
should be done using separate run-once scripts, with the result of the
conversion checked in as the new source.  Some manual work will still
be needed on top.

FWIW it's already possible to do the transform you mention with:

  s/(cc0)/(reg:CC CC_REGNUM_RC)/g

  (define_int_iterator CC_REGNUM_RC [(CC_REGNUM "reload_completed")])

But that's the easy bit to do manually or to script, so I don't think it
really helps.  And IMO adding explicit reload_completed checks would be
better style.

Thanks,
Richard