On 06/11/15 00:56, Segher Boessenkool wrote:
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).
Indeed, this looks better but it still needs the REG_P checks for the inner
operands of the extends to not screw up the arm case.
Here's my proposed extension. I've also modified the testcase slightly to not
use
an unitialised variable. It still demonstrates the issue we're trying to solve.
Bootstrapped and tested on arm and aarch64.
I'll let you put it through it's paces on your setup :)
P.S. Do we want to restrict this to targets that have a widening mul optab like
I did
in the original patch?
Thanks,
Kyrill
2015-11-06 Segher Boessenkool <seg...@kernel.crashing.org>
Kyrylo Tkachov <kyrylo.tkac...@arm.com>
* combine.c (subst): Don't substitute or simplify when
handling register-wise widening multiply.
(force_to_mode): Likewise.
2015-11-06 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
* gcc.target/aarch64/umaddl_combine_1.c: New test.
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. */
commit 24d4a7f9f32e9290e95f940c1ac57fc9be26687c
Author: Kyrylo Tkachov <kyrylo.tkac...@arm.com>
Date: Fri Oct 30 13:56:10 2015 +0000
[combine] Don't transform sign and zero extends inside mults
diff --git a/gcc/combine.c b/gcc/combine.c
index fa1a93f..26b14bf 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5286,6 +5286,22 @@ 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)
+ && REG_P (XEXP (XEXP (x, 0), 0))
+ && REG_P (XEXP (XEXP (x, 1), 0)))
+ {
+ 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')
@@ -8447,6 +8463,17 @@ 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)
+ && REG_P (XEXP (XEXP (x, 0), 0))
+ && REG_P (XEXP (XEXP (x, 1), 0)))
+ 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. */
diff --git a/gcc/testsuite/gcc.target/aarch64/umaddl_combine_1.c b/gcc/testsuite/gcc.target/aarch64/umaddl_combine_1.c
new file mode 100644
index 0000000..430277f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/umaddl_combine_1.c
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mcpu=cortex-a53" } */
+
+enum reg_class
+{
+ NO_REGS,
+ AD_REGS,
+ ALL_REGS, LIM_REG_CLASSES
+};
+
+extern enum reg_class
+ reg_class_subclasses[((int) LIM_REG_CLASSES)][((int) LIM_REG_CLASSES)];
+
+void
+init_reg_sets_1 (unsigned int i)
+{
+ unsigned int j;
+ {
+ for (j = i + 1; j < ((int) LIM_REG_CLASSES); j++)
+ {
+ enum reg_class *p;
+ p = ®_class_subclasses[j][0];
+ while (*p != LIM_REG_CLASSES)
+ p++;
+ }
+ }
+}
+
+/* { dg-final { scan-assembler-not "umull\tw\[0-9\]+, w\[0-9\]+, w\[0-9\]+" } } */
