Re: An alternative fix for PR70944

[Segher Boessenkool]

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

Re: An alternative fix for PR70944

[Richard Sandiford]

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 

Re: An alternative fix for PR70944

[Segher Boessenkool]

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