An alternative fix for PR70944
The transformations made by make_compound_operation apply
only to scalar integer modes. The fix for PR70944 had enforced
that by returning early for vector modes at the top of the
function. However, the function is supposed to be recursive,
so we should continue to look at integer suboperands even if
the outer operation is a vector one.
This patch instead splits out the non-recursive parts
of make_compound_operation into a subroutine and checks
that the mode is a scalar integer before calling it.
The patch was originally written to help with the later
conversion to static type checking of mode classes, but it
also happened to reenable optimisation of things like
vec_duplicate operands.
Note that the gen_lowparts in the PLUS and MINUS cases
were redundant, since new_rtx already had mode "mode"
at those points.
Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
Thanks,
Richard
[ This patch is part of the SVE series posted here:
https://gcc.gnu.org/ml/gcc/2016-11/msg00030.html ]
gcc/
2016-11-15 Richard Sandiford
Alan Hayward
David Sherwood
* combine.c (maybe_swap_commutative_operands): New function.
(combine_simplify_rtx): Use it.
(make_compound_operation_int): New function, split out of...
(make_compound_operation): ...here. Use
maybe_swap_commutative_operands for both.
diff --git a/gcc/combine.c b/gcc/combine.c
index 66f628f..0665f38 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5479,6 +5479,21 @@ subst (rtx x, rtx from, rtx to, int in_dest, int
in_cond, int unique_copy)
return x;
}
+/* If X is a commutative operation whose operands are not in the canonical
+ order, use substitutions to swap them. */
+
+static void
+maybe_swap_commutative_operands (rtx x)
+{
+ if (COMMUTATIVE_ARITH_P (x)
+ && swap_commutative_operands_p (XEXP (x, 0), XEXP (x, 1)))
+{
+ rtx temp = XEXP (x, 0);
+ SUBST (XEXP (x, 0), XEXP (x, 1));
+ SUBST (XEXP (x, 1), temp);
+}
+}
+
/* Simplify X, a piece of RTL. We just operate on the expression at the
outer level; call `subst' to simplify recursively. Return the new
expression.
@@ -5498,13 +5513,7 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int
in_dest,
/* If this is a commutative operation, put a constant last and a complex
expression first. We don't need to do this for comparisons here. */
- if (COMMUTATIVE_ARITH_P (x)
- && swap_commutative_operands_p (XEXP (x, 0), XEXP (x, 1)))
-{
- temp = XEXP (x, 0);
- SUBST (XEXP (x, 0), XEXP (x, 1));
- SUBST (XEXP (x, 1), temp);
-}
+ maybe_swap_commutative_operands (x);
/* Try to fold this expression in case we have constants that weren't
present before. */
@@ -7747,55 +7756,38 @@ extract_left_shift (rtx x, int count)
return 0;
}
-/* Look at the expression rooted at X. Look for expressions
- equivalent to ZERO_EXTRACT, SIGN_EXTRACT, ZERO_EXTEND, SIGN_EXTEND.
- Form these expressions.
-
- Return the new rtx, usually just X.
+/* Subroutine of make_compound_operation. *X_PTR is the rtx at the current
+ level of the expression and MODE is its mode. IN_CODE is as for
+ make_compound_operation. *NEXT_CODE_PTR is the value of IN_CODE
+ that should be used when recursing on operands of *X_PTR.
- Also, for machines like the VAX that don't have logical shift insns,
- try to convert logical to arithmetic shift operations in cases where
- they are equivalent. This undoes the canonicalizations to logical
- shifts done elsewhere.
+ There are two possible actions:
- We try, as much as possible, to re-use rtl expressions to save memory.
+ - Return null. This tells the caller to recurse on *X_PTR with IN_CODE
+ equal to *NEXT_CODE_PTR, after which *X_PTR holds the final value.
- IN_CODE says what kind of expression we are processing. Normally, it is
- SET. In a memory address it is MEM. When processing the arguments of
- a comparison or a COMPARE against zero, it is COMPARE, or EQ if more
- precisely it is an equality comparison against zero. */
+ - Return a new rtx, which the caller returns directly. */
-rtx
-make_compound_operation (rtx x, enum rtx_code in_code)
+static rtx
+make_compound_operation_int (machine_mode mode, rtx *x_ptr,
+enum rtx_code in_code,
+enum rtx_code *next_code_ptr)
{
+ rtx x = *x_ptr;
+ enum rtx_code next_code = *next_code_ptr;
enum rtx_code code = GET_CODE (x);
- machine_mode mode = GET_MODE (x);
int mode_width = GET_MODE_PRECISION (mode);
rtx rhs, lhs;
- enum rtx_code next_code;
- int i, j;
rtx new_rtx = 0;
+ int i;
rtx tem;
- const char *fmt;
bool equality_comparison = false;
- /* PR rtl-optimization/70944. */
- if (VECTOR_MODE_P (mode))
-return x;
-
- /* Select the code to be used in recursive calls. Once we are inside an
- address, we stay there. If we have
Re: An alternative fix for PR70944
On Tue, Nov 15, 2016 at 12:33:06PM +, Richard Sandiford wrote: > The transformations made by make_compound_operation apply > only to scalar integer modes. The fix for PR70944 had enforced > that by returning early for vector modes at the top of the > function. However, the function is supposed to be recursive, > so we should continue to look at integer suboperands even if > the outer operation is a vector one. > > This patch instead splits out the non-recursive parts > of make_compound_operation into a subroutine and checks > that the mode is a scalar integer before calling it. > The patch was originally written to help with the later > conversion to static type checking of mode classes, but it > also happened to reenable optimisation of things like > vec_duplicate operands. > > Note that the gen_lowparts in the PLUS and MINUS cases > were redundant, since new_rtx already had mode "mode" > at those points. > > Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install? Yes, please do. You can use maybe_swap_commutative_operands in change_zero_ext as well, perhaps in more places, do you want to take a look? Thanks, Segher
Re: An alternative fix for PR70944
Segher Boessenkool writes:
> On Tue, Nov 15, 2016 at 12:33:06PM +, Richard Sandiford wrote:
>> The transformations made by make_compound_operation apply
>> only to scalar integer modes. The fix for PR70944 had enforced
>> that by returning early for vector modes at the top of the
>> function. However, the function is supposed to be recursive,
>> so we should continue to look at integer suboperands even if
>> the outer operation is a vector one.
>>
>> This patch instead splits out the non-recursive parts
>> of make_compound_operation into a subroutine and checks
>> that the mode is a scalar integer before calling it.
>> The patch was originally written to help with the later
>> conversion to static type checking of mode classes, but it
>> also happened to reenable optimisation of things like
>> vec_duplicate operands.
>>
>> Note that the gen_lowparts in the PLUS and MINUS cases
>> were redundant, since new_rtx already had mode "mode"
>> at those points.
>>
>> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
>
> Yes, please do. You can use maybe_swap_commutative_operands in
> change_zero_ext as well, perhaps in more places, do you want to
> take a look?
Ah, yeah. change_zero_ext was the only one I could see.
Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
Thanks,
Richard
gcc/
2016-11-15 Richard Sandiford
Alan Hayward
David Sherwood
* combine.c (maybe_swap_commutative_operands): New function.
(combine_simplify_rtx): Use it.
(change_zero_ext): Likewise.
(make_compound_operation_int): New function, split out of...
(make_compound_operation): ...here. Use
maybe_swap_commutative_operands for both.
diff --git a/gcc/combine.c b/gcc/combine.c
index 19ef52f..ca5ddae 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5479,6 +5479,21 @@ subst (rtx x, rtx from, rtx to, int in_dest, int
in_cond, int unique_copy)
return x;
}
+/* If X is a commutative operation whose operands are not in the canonical
+ order, use substitutions to swap them. */
+
+static void
+maybe_swap_commutative_operands (rtx x)
+{
+ if (COMMUTATIVE_ARITH_P (x)
+ && swap_commutative_operands_p (XEXP (x, 0), XEXP (x, 1)))
+{
+ rtx temp = XEXP (x, 0);
+ SUBST (XEXP (x, 0), XEXP (x, 1));
+ SUBST (XEXP (x, 1), temp);
+}
+}
+
/* Simplify X, a piece of RTL. We just operate on the expression at the
outer level; call `subst' to simplify recursively. Return the new
expression.
@@ -5498,13 +5513,7 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int
in_dest,
/* If this is a commutative operation, put a constant last and a complex
expression first. We don't need to do this for comparisons here. */
- if (COMMUTATIVE_ARITH_P (x)
- && swap_commutative_operands_p (XEXP (x, 0), XEXP (x, 1)))
-{
- temp = XEXP (x, 0);
- SUBST (XEXP (x, 0), XEXP (x, 1));
- SUBST (XEXP (x, 1), temp);
-}
+ maybe_swap_commutative_operands (x);
/* Try to fold this expression in case we have constants that weren't
present before. */
@@ -7744,55 +7753,38 @@ extract_left_shift (rtx x, int count)
return 0;
}
-/* Look at the expression rooted at X. Look for expressions
- equivalent to ZERO_EXTRACT, SIGN_EXTRACT, ZERO_EXTEND, SIGN_EXTEND.
- Form these expressions.
-
- Return the new rtx, usually just X.
+/* Subroutine of make_compound_operation. *X_PTR is the rtx at the current
+ level of the expression and MODE is its mode. IN_CODE is as for
+ make_compound_operation. *NEXT_CODE_PTR is the value of IN_CODE
+ that should be used when recursing on operands of *X_PTR.
- Also, for machines like the VAX that don't have logical shift insns,
- try to convert logical to arithmetic shift operations in cases where
- they are equivalent. This undoes the canonicalizations to logical
- shifts done elsewhere.
+ There are two possible actions:
- We try, as much as possible, to re-use rtl expressions to save memory.
+ - Return null. This tells the caller to recurse on *X_PTR with IN_CODE
+ equal to *NEXT_CODE_PTR, after which *X_PTR holds the final value.
- IN_CODE says what kind of expression we are processing. Normally, it is
- SET. In a memory address it is MEM. When processing the arguments of
- a comparison or a COMPARE against zero, it is COMPARE, or EQ if more
- precisely it is an equality comparison against zero. */
+ - Return a new rtx, which the caller returns directly. */
-rtx
-make_compound_operation (rtx x, enum rtx_code in_code)
+static rtx
+make_compound_operation_int (machine_mode mode, rtx *x_ptr,
+enum rtx_code in_code,
+enum rtx_code *next_code_ptr)
{
+ rtx x = *x_ptr;
+ enum rtx_code next_code = *next_code_ptr;
enum rtx_code code = GET_CODE (x);
- machine_mode mode = GET_MODE (x);
int mode_width = GET_MODE_PRECISION (m
Re: An alternative fix for PR70944
On Wed, Nov 16, 2016 at 10:16:16AM +, Richard Sandiford wrote: > Segher Boessenkool writes: > > On Tue, Nov 15, 2016 at 12:33:06PM +, Richard Sandiford wrote: > >> The transformations made by make_compound_operation apply > >> only to scalar integer modes. The fix for PR70944 had enforced > >> that by returning early for vector modes at the top of the > >> function. However, the function is supposed to be recursive, > >> so we should continue to look at integer suboperands even if > >> the outer operation is a vector one. > >> > >> This patch instead splits out the non-recursive parts > >> of make_compound_operation into a subroutine and checks > >> that the mode is a scalar integer before calling it. > >> The patch was originally written to help with the later > >> conversion to static type checking of mode classes, but it > >> also happened to reenable optimisation of things like > >> vec_duplicate operands. > >> > >> Note that the gen_lowparts in the PLUS and MINUS cases > >> were redundant, since new_rtx already had mode "mode" > >> at those points. > >> > >> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install? > > > > Yes, please do. You can use maybe_swap_commutative_operands in > > change_zero_ext as well, perhaps in more places, do you want to > > take a look? > > Ah, yeah. change_zero_ext was the only one I could see. > > Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install? Yes, still okay. Thanks! Segher
