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

[David Edelsohn <dje dot gcc at gmail dot com>]

On Fri, May 23, 2014 at 11:23 AM, Alan Modra <amodra@gmail.com> wrote:
> OK, let's start again from scratch.  This patch fixes PR61098, a
> problem caused by trying to do arithmetic on the count register.  The
> fix is to provide a new pseudo in rs6000_emit_set_long_const so
> arithmetic will be done in a gpr.
>
> Additionally, the patch fixes a number of other bugs and cleanup
> issues with rs6000_emit_set_{,long_}const.
>
> - rs6000_emit_set_long_const took two HWI constant parameters, a relic
>   from the days when HWI might be 32 bits on powerpc.  We're only
>   setting a 64-bit value, so remove the unnecessary parameter.
>
> - The !TARGET_POWERPC64 handling of DImode assumed a 32 bit HWI, and
>   the insn setting the low 32-bit reg was wrongly marked with a
>   reg_equiv note saying the reg contained the entire 64-bit constant.
>   I hadn't spotted the bad reg_equiv when writing the previous patch.
>
> - The comments describing the functions are inaccurate and misleading.
>
> - rs6000_emit_set_long_const always returns DEST, so it's caller can
>   assume this and rs6000_emit_set_long_const return void.
>
> - The code testing for a NULL DEST in rs6000_emit_set_const is dead.
>   DEST cannot be NULL, since the three uses of the function are in
>   rs6000.md splitters where DEST (operand[0]) satisfies
>   gpc_reg_operand.
>
> - The above two points mean that rs6000_emit_set_const always returns
>   DEST, which in turn would allow rs6000_emit_set_const to return
>   void.  However, in view of a possible future change that might need
>   to return status on whether rs6000_emit_set_const emitted anything,
>   return a bool.
>
> - rs6000_emit_set_const parameter N is currently unused, and MODE
>   always matches GET_MODE (DEST), so N and MODE can be removed.
>
> - The code is liberally sprinkled with copy_rtx.  DEST/TEMP is always
>   used once without copy_rtx, but which insn uses copy_rtx varies.  I
>   changed the code to always use a bare DEST as the last insn for
>   consistency.  (Writing the code this way might allow us to omit the
>   copy_rtx on DEST/TEMP entirely.  Before reload TEMP will be a new
>   pseudo reg, thus doesn't need copy_rtx, and after reload we
>   shouldn't have a SUBREG DEST.  I wasn't sure of exactly what might
>   happen during reload, so left well enough alone.)
>
> Bootstrapped and regression tested powerpc64-linux.  OK to apply
> mainline?

This is a much clearer start. Thanks for the revised version.

This is okay.

Thanks, David