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
