This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix wrong code on aarch64 due to paradoxical subreg


On 07/26/2016 11:32 AM, Bernd Edlinger wrote:
Hi!

As described in PR 71779 and PR 70903 we have a wrong code issue on aarch64
which is triggered by a paradoxical subreg that can be created in
store_bit_field_using_insv when the target has an EP_insv bitfield instruction.

The attached patch changes this insn...

(insn 1047 1046 1048 (set (reg:DI 481)
        (subreg:DI (reg/f:SI 479) 0)) y.c:12702 -1
     (nil))

... into a more simple insn:

(insn 1047 1046 1048 (set (reg:DI 479)
        (zero_extend:DI (reg:SI 480))) isl_input.c:2496 -1
     (nil))

which manages to fix two bugs at the same time.

The patch does not include a test case, as I was looking at the PR 71779
just by curiosity, and I have only a theoretical interest in aarch64. However,
it should be easy to extract one from PR 70903 at least, but I can't do that by
myself.

Therefore I would like to leave the test case(s) to Cristina Tamar from ARM,
and/or Andreas Schwab, who have also helped out with bootstrapping and
reg-testing on aarch64 and aarch64-ilp32.

Bootstrap and reg-test on x86_64-linux-gnu with no regressions.
Successfully bootstrapped on aarch64_ilp32 and the ISL testsuite passes.
Also successfully bootstrapped on an aarch64 juno board and no regressions.


Is it OK for trunk?


Thanks
Bernd.


patch-pr71779.diff


2016-07-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR rtl-optimization/71779
	PR rtl-optimization/70903
	* expmed.c (store_bit_field_using_insv): Don't generate a paradoxical
	subreg.
So you're stumbling into another really interesting area.

I can hazard a guess that the reason we create the paradoxical SUBREG rather than a zero or sign extension is because various optimizers know that the upper bits in a paradoxical are don't care bits and may make transformations with that knowledge.

Once you turn that into a zero/sign extend, then the rest of the optimizers must preserve the zero/sign extension property -- they have no way of knowing the bits were don't cares.

So it's not necessarily clear that your change results in generally better code or not.

More importantly, it really feels like you're papering over a real bug elsewhere. AFAICT the use of a paradoxical subreg here is safe. So the real bug ought to be downstream of this point. Several folks have pointed at reload, which would certainly be a point ripe for a paradoxical subreg problem.

Sooo. I don't think we can go with this patch as-is today. We need to find the root problem which I believe is later in the pipeline.

This patch might make sense from an optimization standpoint -- I'm generally of the opinion that while paradoxical subregs give the optimziers more freedom, the optimizers rarely are able to use it and they generally know much more about the semantics of and how to optimize around zero/sign extensions. But we really should fix the bug first, then come back to improving the code generation.

Now if someone wanted to look for an interesting optimization project -- ree.c knows how to do redundant extension eliminations. A paradoxical is a form of extension that isn't currently understood by REE. Similarly REE doesn't know how to handle zero-extension as an "and" or sign extension as a pair of shifts. Both of which can occur in practice due to costs or ISA limitations. I wouldn't be surprise if adding those capabilities to REE would significantly improve its ability to eliminate unnecessary extensions.

Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]