Re: [RS6000] Fix PR61098, Poor code setting count register

[David Edelsohn <dje.gcc@gmail.com>]

On Thu, May 8, 2014 at 10:40 PM, Alan Modra <amodra@gmail.com> wrote:

>> Please do not remove all of the comments from the two functions. The
>> comments should provide some documentation about the different
>> purposes of the two functions other than setting DEST to a CONST.
>
> I believe my updated comment covers the complete purpose of the
> function nowadays.  The comments I removed are out-dated, and should
> have been removed a long time ago..  rs6000_emit_set_const does not
> even look at N, it always returns a non-zero result, and the return is
> only tested for non-zero.  I removed MODE too, because that is always
> the same as GET_MODE (dest).

It is helpful if the comment expresses more than restating the
information one can glean from the function name. It's useful to note
that rs6000_emit_set_long_const is a standard decomposition with a
bounded number of instructions.

>> I think that the way you rearranged the invocations of copy_rtx() in
>> rs6000_emit_set_long_const() is okay, but it would be good for someone
>> else to double check.
>
> Yeah, that function is a bit messy.  I took the approach of always use
> a bare "dest" once in the last instruction emitted, with every other
> use getting hit with copy_rtx.  The previous approach was similar,
> but used the bare "dest" on the first instruction emitted.  Obviously
> you don't need copy_rtx anywhere with the new code when
> can_create_pseudo_p is true, but I felt it wasn't worth optimising
> that for the added source complication.

Can you help clarify the removal of the code that tests if the
splitter failed?  The splitters in the Alpha port follow mostly the
same rhythm, with a little bit of further cleanup and consolidation
relative to the rs6000 port. alpha_split_const_mov() falls back on
alpha_emit_set_long_const(), but checks that the target is valid and
allows the splitter to fail. Either the Alpha port is doing
unnecessary work or this cleanup patch is too aggressive. Either way,
a comment seems necessary.

Thanks, David