On Thu, Nov 05, 2015 at 12:01:26PM +0000, Kyrill Tkachov wrote:
> Thanks, that looks less intrusive. I did try it out on arm and aarch64.
> It does work on the aarch64 testcase. However, there's also a correctness
> regression, I'll try to explain inline....
> >diff --git a/gcc/combine.c b/gcc/combine.c
> >index c3db2e0..3bf7ffb 100644
> >--- a/gcc/combine.c
> >+++ b/gcc/combine.c
> >@@ -5284,6 +5284,15 @@ subst (rtx x, rtx from, rtx to, int in_dest, int
> >in_cond, int unique_copy)
> > || GET_CODE (SET_DEST (x)) == PC))
> > fmt = "ie";
> >
> >+ /* Substituting into the operands of a widening MULT is not likely
> >+ to create RTL matching a machine insn. */
> >+ if (code == MULT
> >+ && (GET_CODE (XEXP (x, 0)) == ZERO_EXTEND
> >+ || GET_CODE (XEXP (x, 0)) == SIGN_EXTEND)
> >+ && (GET_CODE (XEXP (x, 1)) == ZERO_EXTEND
> >+ || GET_CODE (XEXP (x, 1)) == SIGN_EXTEND))
> >+ return x;
>
> I think we should also add:
> && REG_P (XEXP (XEXP (x, 0), 0))
> && REG_P (XEXP (XEXP (x, 1), 0))
>
> to the condition. Otherwise I've seen regressions in the arm testsuite, the
> gcc.target/arm/smlatb-1.s test in particular that tries to match the pattern
> (define_insn "*maddhisi4tb"
> [(set (match_operand:SI 0 "s_register_operand" "=r")
> (plus:SI (mult:SI (ashiftrt:SI
> (match_operand:SI 1 "s_register_operand" "r")
> (const_int 16))
> (sign_extend:SI
> (match_operand:HI 2 "s_register_operand" "r")))
> (match_operand:SI 3 "s_register_operand" "r")))]
>
>
> There we have a sign_extend of a shift that we want to convert to the form
> that the pattern expects. So adding the checks for REG_P fixes that for me.
I'll have a look at this; I thought it should be handled with the new patch
(attached), but maybe not.
> For the correctness issue I saw on aarch64 the shortest case I could reduce
> is:
> short int a[16], b[16];
> void
> f5 (void)
> {
> a[0] = b[0] / 14;
> }
(Without -mcpu=cortex-a53, or you just get a divide insn).
> Is there a way that subst can signal some kind of "failed to substitute"
> result?
Yep, see new patch. The "from == to" condition is for when subst is called
just to simplify some code (normally with pc_rtx, pc_rtx).
> If not, I got it to work by using that check to set the in_dest variable to
> the
> subsequent recursive call to subst, in a similar way to my original patch,
> but I was
> hoping to avoid overloading the meaning of in_dest.
Yes me too :-)
Segher
diff --git a/gcc/combine.c b/gcc/combine.c
index c3db2e0..c806db9 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5284,6 +5284,20 @@ subst (rtx x, rtx from, rtx to, int in_dest, int
in_cond, int unique_copy)
|| GET_CODE (SET_DEST (x)) == PC))
fmt = "ie";
+ /* Substituting into the operands of a widening MULT is not likely
+ to create RTL matching a machine insn. */
+ if (code == MULT
+ && (GET_CODE (XEXP (x, 0)) == ZERO_EXTEND
+ || GET_CODE (XEXP (x, 0)) == SIGN_EXTEND)
+ && (GET_CODE (XEXP (x, 1)) == ZERO_EXTEND
+ || GET_CODE (XEXP (x, 1)) == SIGN_EXTEND))
+ {
+ if (from == to)
+ return x;
+ else
+ return gen_rtx_CLOBBER (GET_MODE (x), const0_rtx);
+ }
+
/* Get the mode of operand 0 in case X is now a SIGN_EXTEND of a
constant. */
if (fmt[0] == 'e')
@@ -8455,6 +8469,15 @@ force_to_mode (rtx x, machine_mode mode, unsigned
HOST_WIDE_INT mask,
/* ... fall through ... */
case MULT:
+ /* Substituting into the operands of a widening MULT is not likely to
+ create RTL matching a machine insn. */
+ if (code == MULT
+ && (GET_CODE (XEXP (x, 0)) == ZERO_EXTEND
+ || GET_CODE (XEXP (x, 0)) == SIGN_EXTEND)
+ && (GET_CODE (XEXP (x, 1)) == ZERO_EXTEND
+ || GET_CODE (XEXP (x, 1)) == SIGN_EXTEND))
+ return gen_lowpart_or_truncate (mode, x);
+
/* For PLUS, MINUS and MULT, we need any bits less significant than the
most significant bit in MASK since carries from those bits will
affect the bits we are interested in. */
--
1.9.3
