Re: [PATCH #2, rs6000][GCC6] Fix PR78543, ICE in push_reload on powerpc64le-linux-gnu

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi Mike,

On Fri, Mar 24, 2017 at 07:23:02PM -0400, Michael Meissner wrote:
> Reload fumbles in certain conditions.

Yeah.  And it does not need bswap to get totally lost with this, so this
patch is a workaround, not a fix.

It does make things nicer though :-)

> LRA generates working code, but the
> store and the load with byte reverse from the same location, can slow things
> down compared to the operation on registers.

How so?  You mean (say) lwbrx after doing a stw to that same address?
We have that problem in general :-/


Thanks for the extensive testing.

> Can I apply the patch to the GCC 7 trunk?  Since the patch shows up as an abort
> in GCC 6 and not in GCC 7 (unless you disable LRA), I would like to apply the
> patch as soon as possible to the GCC 6 branch.

A few remarks:

> +; Types that have hardware load/store with byte reverse
> +(define_mode_iterator BSWAP [HI
> +			     SI
> +			     (DI "TARGET_POWERPC64 && TARGET_LDBRX")])

The patch would be much easier to read if you had done this step (with HSI
as well) separately.  Oh well.

> +(define_expand "bswap<mode>2"

> +  if (MEM_P (src))
> +    emit_insn (gen_bswap<mode>2_load (dest, src));
> +  else
> +    {
> +      if (MEM_P (dest))
> +	emit_insn (gen_bswap<mode>2_store (dest, src));
> +      else
> +	emit_insn (gen_bswap<mode>2_reg (dest, src));
> +    }
> +  DONE;

Please avoid the extra nesting, i.e. do

+  if (MEM_P (src))
+    emit_insn (gen_bswap<mode>2_load (dest, src));
+  else if (MEM_P (dest))
+    emit_insn (gen_bswap<mode>2_store (dest, src));
+  else
+    emit_insn (gen_bswap<mode>2_reg (dest, src));
+  DONE;

(also for bswapdi2).

> +  [(set_attr "length" "12")
> +   (set_attr "type" "*")])

Lose that last line?  You don't need to explicitly set things to their
default value ;-)
OTOH, you might want to make it type "three" instead?

> +  [(set_attr "length" "36")
> +   (set_attr "type" "*")])

Same.

This patch is okay for trunk.  Also for 6, but what is the hurry there?

Thanks!


Segher