[PATCH] combine: Improve change_zero_ext (fixes PR71847)
Georg-Johann Lay
avr@gjlay.de
Wed Nov 23 15:58:00 GMT 2016
Hi, this causes an illegal code issue on avr.
Test case (reduced from gcc.dg/builtins-32.c):
extern int signbitf (float);
int test (float x)
{
return signbitf (x);
}
Before combine, the dump reads
(note 4 0 19 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
(insn 19 4 3 2 (set (reg:QI 51 [ x+3 ])
(reg:QI 25 r25 [ x+3 ])) "builtins-32.c":4 56 {movqi_insn}
(expr_list:REG_DEAD (reg:QI 25 r25 [ x+3 ])
(nil)))
(note 3 19 7 2 NOTE_INSN_FUNCTION_BEG)
(insn 7 3 8 2 (set (reg:HI 45 [ x+3 ])
(zero_extend:HI (reg:QI 51 [ x+3 ]))) "builtins-32.c":5 370
{zero_extendqihi2}
and then the .combine dump comes up with:
Trying 19 -> 7:
Failed to match this instruction:
(set (reg:HI 45 [ x+3 ])
(zero_extend:HI (reg:QI 25 r25 [ x+3 ])))
Successfully matched this instruction:
(set (reg:HI 45 [ x+3 ])
(and:HI (reg:HI 25 r25)
(const_int 255 [0xff])))
allowing combination of insns 19 and 7
...
Trying 8 -> 13:
Successfully matched this instruction:
(parallel [
(set (reg/i:HI 24 r24)
(and:HI (reg:HI 25 r25)
(const_int 128 [0x80])))
(clobber (scratch:QI))
])
R25 is a 16-bit hard reg, but HImode (and larger) registers must start
at an even register number.
HARD_REGNO_MODE_OK rejects such registers, and reload then generates a
new movhi insn to fix the andhi3. The .reload dump reads:
(insn 22 8 13 2 (set (reg/i:HI 24 r24)
(reg:HI 25 r25)) "builtins-32.c":6 68 {*movhi}
(nil))
(insn 13 22 14 2 (parallel [
(set (reg/i:HI 24 r24)
(and:HI (reg/i:HI 24 r24)
(const_int 128 [0x80])))
(clobber (scratch:QI))
]) "builtins-32.c":6 251 {andhi3}
(nil))
This HI move is obviously wrong, persists until asm out and then gas
complains, of course:
builtins-32.s: Assembler messages:
builtins-32.s:15: Error: even register number required
http://gcc.gnu.org/r241664 is the first revision that exposes the problem.
Command line used is
$ avr-gcc builtins-32.c -mmcu=avr5 -Os -c -save-temps -dp -da
and I configured with
$ ../../gcc.gnu.org/trunk/configure --target=avr
--prefix=/local/gnu/install/gcc-7 --disable-shared --disable-nls
--with-dwarf2 --enable-target-optspace=yes --with-gnu-as --with-gnu-ld
--enable-checking=release --enable-languages=c,c++,lto
Ideas?
The problem is that reload cannot actually fix the situation. The
andhi3 is the only insn in the function, and r25 is an incoming register
("x" is passed in QI:r22 .. QI:r25), but after the change from combine
it appears as if QI:r26 is also an incoming register as r25 das HImode
thereafter!
Johann
On 25.10.2016 12:40, Segher Boessenkool wrote:
> This improves a few things in change_zero_ext. Firstly, it should use
> the passed in pattern in recog_for_combine, not the pattern of the insn
> (they are not the same if the whole pattern was replaced). Secondly,
> it handled zero_ext of a subreg, but with hard registers we do not get
> a subreg, instead the mode of the reg is changed. So this handles that.
> Thirdly, after changing a zero_ext to an AND, the resulting RTL may become
> non-canonical, like (ior (ashift ..) (and ..)); the AND should be first,
> it is commutative. And lastly, zero_extract as a set_dest wasn't handled
> at all, but now it is.
>
> This fixes the testcase in PR71847, and improves code generation in some
> other edge cases too.
>
> Tested on powerpc64-linux {-m32,-m64}; I'll also test it on x86 and
> will build lots of crosses before committing.
>
>
> Segher
>
>
> 2016-10-25 Segher Boessenkool <segher@kernel.crashing.org>
>
> PR target/71847
> * combine.c (change_zero_ext): Handle zero_ext of hard registers.
> Swap commutative operands in new RTL if needed. Handle zero_ext
> in the set_dest.
> (recog_for_combine): Pass *pnewpat to change_zero_ext instead of
> PATTERN (insn).
>
> ---
> gcc/combine.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/combine.c b/gcc/combine.c
> index ee12714..b46405b 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -11140,9 +11140,10 @@ recog_for_combine_1 (rtx *pnewpat, rtx_insn *insn, rtx *pnotes)
> Return whether anything was so changed. */
>
> static bool
> -change_zero_ext (rtx *src)
> +change_zero_ext (rtx pat)
> {
> bool changed = false;
> + rtx *src = &SET_SRC (pat);
>
> subrtx_ptr_iterator::array_type array;
> FOR_EACH_SUBRTX_PTR (iter, array, src, NONCONST)
> @@ -11174,6 +11175,14 @@ change_zero_ext (rtx *src)
> size = GET_MODE_PRECISION (GET_MODE (XEXP (x, 0)));
> x = SUBREG_REG (XEXP (x, 0));
> }
> + else if (GET_CODE (x) == ZERO_EXTEND
> + && SCALAR_INT_MODE_P (mode)
> + && REG_P (XEXP (x, 0))
> + && HARD_REGISTER_P (XEXP (x, 0)))
> + {
> + size = GET_MODE_PRECISION (GET_MODE (XEXP (x, 0)));
> + x = gen_rtx_REG (mode, REGNO (XEXP (x, 0)));
> + }
> else
> continue;
>
> @@ -11184,6 +11193,44 @@ change_zero_ext (rtx *src)
> changed = true;
> }
>
> + if (changed)
> + FOR_EACH_SUBRTX_PTR (iter, array, src, NONCONST)
> + {
> + rtx x = **iter;
> + if (COMMUTATIVE_ARITH_P (x)
> + && swap_commutative_operands_p (XEXP (x, 0), XEXP (x, 1)))
> + {
> + rtx tem = XEXP (x, 0);
> + SUBST (XEXP (x, 0), XEXP (x, 1));
> + SUBST (XEXP (x, 1), tem);
> + }
> + }
> +
> + rtx *dst = &SET_DEST (pat);
> + if (GET_CODE (*dst) == ZERO_EXTRACT
> + && REG_P (XEXP (*dst, 0))
> + && CONST_INT_P (XEXP (*dst, 1))
> + && CONST_INT_P (XEXP (*dst, 2)))
> + {
> + rtx reg = XEXP (*dst, 0);
> + int width = INTVAL (XEXP (*dst, 1));
> + int offset = INTVAL (XEXP (*dst, 2));
> + machine_mode mode = GET_MODE (reg);
> + int reg_width = GET_MODE_PRECISION (mode);
> + if (BITS_BIG_ENDIAN)
> + offset = reg_width - width - offset;
> +
> + wide_int mask = wi::shifted_mask (offset, width, true, reg_width);
> + rtx x = gen_rtx_AND (mode, reg, immed_wide_int_const (mask, mode));
> + rtx y = simplify_gen_binary (ASHIFT, mode, SET_SRC (pat),
> + GEN_INT (offset));
> + rtx z = simplify_gen_binary (IOR, mode, x, y);
> + SUBST (SET_DEST (pat), reg);
> + SUBST (SET_SRC (pat), z);
> +
> + changed = true;
> + }
> +
> return changed;
> }
>
> @@ -11206,7 +11253,7 @@ change_zero_ext (rtx *src)
> static int
> recog_for_combine (rtx *pnewpat, rtx_insn *insn, rtx *pnotes)
> {
> - rtx pat = PATTERN (insn);
> + rtx pat = *pnewpat;
> int insn_code_number = recog_for_combine_1 (pnewpat, insn, pnotes);
> if (insn_code_number >= 0 || check_asm_operands (pat))
> return insn_code_number;
> @@ -11215,7 +11262,7 @@ recog_for_combine (rtx *pnewpat, rtx_insn *insn, rtx *pnotes)
> bool changed = false;
>
> if (GET_CODE (pat) == SET)
> - changed = change_zero_ext (&SET_SRC (pat));
> + changed = change_zero_ext (pat);
> else if (GET_CODE (pat) == PARALLEL)
> {
> int i;
> @@ -11223,7 +11270,7 @@ recog_for_combine (rtx *pnewpat, rtx_insn *insn, rtx *pnotes)
> {
> rtx set = XVECEXP (pat, 0, i);
> if (GET_CODE (set) == SET)
> - changed |= change_zero_ext (&SET_SRC (set));
> + changed |= change_zero_ext (set);
> }
> }
>
>
More information about the Gcc-patches
mailing list