Peter Bergner - Re: [PATCH] Fix PR middle-end/28690, indexed load/store performance + re (original) (raw)

This is the mail archive of the gcc-patches@gcc.gnu.orgmailing list for the GCC project.

• From: Peter Bergner
• To: Richard Henderson , Jan Hubicha
• Cc: gcc-patches at gcc dot gnu dot org, Paolo Bonzini , David Edelsohn
• Date: Mon, 12 Feb 2007 12:07:53 -0600
• Subject: Re: [PATCH] Fix PR middle-end/28690, indexed load/store performance + reload bug
• References: 20061205050808.GA13002@vervain.rchland.ibm.com

Richard / Jan,

Can either of you please explain the reasoning behind the change to optabs.c's emit_cmp_and_jump_insn() in the following post?

http://gcc.gnu.org/ml/gcc-patches/2001-05/msg01496.html

/* Swap operands and condition to ensure canonical RTL. / if (swap_commutative_operands_p (x, y)) { / If we're not emitting a branch, this means some caller is out of sync. */ if (! label) abort ();

  op0 = y, op1 = x;
  comparison = swap_condition (comparison);
}

Specifically, I'm wondering about the abort() you added which has since been changed to a "gcc_assert (label)". I'm asking, because I'm hitting this abort with the patch I'm testing to fix PR28690. The patch, which I have appended below, modifies commutative_operand_precedence() so that REG_POINTER and MEM_POINTER operands are preferred over non pointers, so that indexed load/store instructions on the POWER6 processor will get the base pointer in the correct position within the instruction (this patch gives us a 30% overall improvement on SPEC2000 and 500% on galgel). The problem is, this patch causes us to ICE on the gcc_assert (label) within emit_cmp_and_jump_insn() when we try and swap 2 REG's because REGNO(y) is less than REGNO(x).

Is this abort/gcc_assert really guarding a bug from happening or is it something you just don't want to happen but could live with? I eliminated the abort/gcc_assert and bootstrapped/regtested on powerpc64-linux with no ill effects, so maybe this is only needed for Alpha?

Thanks.

Peter

Index: optabs.c

--- optabs.c (revision 121186) +++ optabs.c (working copy) @@ -1193,29 +1193,6 @@ expand_simple_binop (enum machine_mode m return expand_binop (mode, binop, op0, op1, target, unsignedp, methods); } -/* Return whether OP0 and OP1 should be swapped when expanding a commutative - binop. Order them according to commutative_operand_precedence and, if - possible, try to put TARGET or a pseudo first. */ -static bool -swap_commutative_operands_with_target (rtx target, rtx op0, rtx op1) -{ - int op0_prec = commutative_operand_precedence (op0); - int op1_prec = commutative_operand_precedence (op1);

• if (op0_prec < op1_prec)
• return true;
• if (op0_prec > op1_prec)
• return false;
• /* With equal precedence, both orders are ok, but it is better if the

• first operand is TARGET, or if both TARGET and OP0 are pseudos.  */

• if (target == 0 || REG_P (target))
• return (REG_P (op1) && !REG_P (op0)) || target == op1;
• else
• return rtx_equal_p (op1, target);

-}

/* Generate code to perform an operation specified by BINOPTAB on operands OP0 and OP1, with result having machine-mode MODE. @@ -1292,7 +1269,7 @@ expand_binop (enum machine_mode mode, op { commutative_op = 1; - if (swap_commutative_operands_with_target (target, op0, op1)) + if (swap_commutative_operands_p (op0, op1)) { temp = op1; op1 = op0; Index: rtlanal.c

--- rtlanal.c (revision 121186) +++ rtlanal.c (working copy) @@ -2776,9 +2776,9 @@ commutative_operand_precedence (rtx op)
/* Constants always come the second operand. Prefer "nice" constants. / if (code == CONST_INT) - return -7; + return -10; if (code == CONST_DOUBLE) - return -6; + return -9; op = avoid_constant_pool_reference (op); code = GET_CODE (op); @@ -2786,26 +2786,29 @@ commutative_operand_precedence (rtx op) { case RTX_CONST_OBJ: if (code == CONST_INT) - return -5; + return -8; if (code == CONST_DOUBLE) - return -4; - return -3; + return -7; + return -6; case RTX_EXTRA: / SUBREGs of objects should come second. / if (code == SUBREG && OBJECT_P (SUBREG_REG (op))) - return -2; + return -5; if (!CONSTANT_P (op)) return 0; else / As for RTX_CONST_OBJ. / - return -3; + return -6; case RTX_OBJ: / Complex expressions should be the first, so decrease priority of objects. / - return -1; + if (REG_P (op)) + return (REG_POINTER (op)) ? -1 : -3; + else + return (MEM_P (op) && MEM_POINTER (op)) ? -2 : -4; case RTX_COMM_ARITH: / Prefer operands that are themselves commutative to be first. @@ -2832,11 +2835,19 @@ commutative_operand_precedence (rtx op) /* Return 1 iff it is necessary to swap operands of commutative operation in order to canonicalize expression. / -int +bool swap_commutative_operands_p (rtx x, rtx y) { - return (commutative_operand_precedence (x) - < commutative_operand_precedence (y)); + int result = (commutative_operand_precedence (x) + - commutative_operand_precedence (y)); + if (result) + return result < 0; + + /* Group together equal REGs to do more simplification. */ + if (REG_P (x) && REG_P (y)) + return REGNO (x) > REGNO (y); + + return false; } / Return 1 if X is an autoincrement side effect and the register is Index: tree-ssa-address.c

--- tree-ssa-address.c (revision 121186) +++ tree-ssa-address.c (working copy) @@ -125,7 +125,7 @@ gen_addr_rtx (rtx symbol, rtx base, rtx if (base) { if (*addr) - *addr = gen_rtx_PLUS (Pmode, *addr, base); + *addr = simplify_gen_binary (PLUS, Pmode, base, *addr); else *addr = base; } Index: rtl.h

--- rtl.h (revision 121186) +++ rtl.h (working copy) @@ -1680,7 +1680,7 @@ extern int reg_referenced_p (rtx, rtx); extern int reg_used_between_p (rtx, rtx, rtx); extern int reg_set_between_p (rtx, rtx, rtx); extern int commutative_operand_precedence (rtx); -extern int swap_commutative_operands_p (rtx, rtx); +extern bool swap_commutative_operands_p (rtx, rtx); extern int modified_between_p (rtx, rtx, rtx); extern int no_labels_between_p (rtx, rtx); extern int modified_in_p (rtx, rtx);

• Follow-Ups:
  • Re: [PATCH] Fix PR middle-end/28690, indexed load/store performance + reload bug
    * From: Richard Henderson