PR/23303 (not using leal on ix86)

PR/23303 (not using leal on ix86)

[Paolo Bonzini]

It looks like Jan's patch at 
http://gcc.gnu.org/ml/gcc-patches/2005-07/msg02021.html is causing a 
code size regression in i686.  I'll consider a 32-bit target, and this 
testcase

   char **
   VTallocbuf(char **allbuf, unsigned long savelines)
   {
     return &allbuf[savelines];
   }

For i686 we produce

         movl    8(%esp), %eax
         movl    4(%esp), %edx
         sall    $2, %eax
         addl    %edx, %eax

instead of

         movl    8(%esp), %eax
         movl    4(%esp), %edx
         leal    (%edx,%eax,4), %eax

However even in this case a no-lea code could be feasible:

         movl    8(%esp), %eax
         sall    $2, %eax
         addl    4(%esp), %eax

And this is exactly the rtl we have until peephole2, where this peephole 
splits the instruction:

;; Don't do logical operations with memory inputs.
(define_peephole2
   [(match_scratch:SI 2 "r")
    (parallel [(set (match_operand:SI 0 "register_operand" "")
                    (match_operator:SI 3 "arith_or_logical_operator"
                      [(match_dup 0)
                       (match_operand:SI 1 "memory_operand" "")]))
               (clobber (reg:CC FLAGS_REG))])]
   "! optimize_size && ! TARGET_READ_MODIFY"
   [(set (match_dup 2) (match_dup 1))
    (parallel [(set (match_dup 0)
                    (match_op_dup 3 [(match_dup 0) (match_dup 2)]))
               (clobber (reg:CC FLAGS_REG))])]
   "")

I think that Jan's patch should be conditionalized: if !optimize_size && 
!TARGET_READ_MODIFY, the transformation he removed will be done anyway, 
and too late in the game.

Let's see what the hunks do...

Index: expr.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.c,v
retrieving revision 1.806
diff -c -3 -p -r1.806 expr.c
*** expr.c	25 Jul 2005 12:04:45 -0000	1.806
--- expr.c	29 Jul 2005 12:10:40 -0000
*************** expand_expr_real_1 (tree exp, rtx target
*** 6578,6595 ****
         target = 0;
       }

-   /* If will do cse, generate all results into pseudo registers
-      since 1) that allows cse to find more things
-      and 2) otherwise cse could produce an insn the machine
-      cannot support.  An exception is a CONSTRUCTOR into a multi-word
-      MEM: that's much more likely to be most efficient into the MEM.
-      Another is a CALL_EXPR which must return in memory.  */
-
-   if (! cse_not_expected && mode != BLKmode && target
-       && (!REG_P (target) || REGNO (target) < FIRST_PSEUDO_REGISTER)
-       && ! (code == CONSTRUCTOR && GET_MODE_SIZE (mode) > UNITS_PER_WORD)
-       && ! (code == CALL_EXPR && aggregate_value_p (exp, exp)))
-     target = 0;

     switch (code)
       {

I think this ought to be left in.  Not because CSE can find more things, 
but because in general an instruction selection pass ought to recombine 
MEMs at their usage points if it is worthwhile.  To this end, 
ix86_rtx_costs could be taught about TARGET_READ_MODIFY, with something 
like:

    case MEM:
      if (!optimize && !TARGET_READ_MODIFY
          && GET_RTX_CLASS (outer_code) == RTX_BIN_ARITH)
        *total++;
      break;

Also consider that we still lack tree-based load PRE (PR23455), so we 
still need CSE to "find more things".  Load PRE is the major obstacle 
towards removing CSE path following.

--- 6578,6583 ----
Index: optabs.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/optabs.c,v
retrieving revision 1.287
diff -c -3 -p -r1.287 optabs.c
*** optabs.c	12 Jul 2005 09:20:02 -0000	1.287
--- optabs.c	29 Jul 2005 12:10:41 -0000
*************** expand_vec_cond_expr (tree vec_cond_expr
*** 5475,5481 ****
     if (icode == CODE_FOR_nothing)
       return 0;

!   if (!target)
       target = gen_reg_rtx (mode);

     /* Get comparison rtx.  First expand both cond expr operands.  */
--- 5475,5481 ----
     if (icode == CODE_FOR_nothing)
       return 0;

!   if (!target || !insn_data[icode].operand[0].predicate (target, mode))
       target = gen_reg_rtx (mode);

     /* Get comparison rtx.  First expand both cond expr operands.  */

This is partially unrelated, it does not hurt.

Index: config/i386/i386.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/i386/i386.c,v
retrieving revision 1.843
diff -c -3 -p -r1.843 i386.c
*** config/i386/i386.c	18 Jul 2005 06:39:18 -0000	1.843
--- config/i386/i386.c	29 Jul 2005 12:10:42 -0000
*************** ix86_fixup_binary_operands (enum rtx_cod
*** 8154,8170 ****
         && GET_RTX_CLASS (code) != RTX_COMM_ARITH)
       src1 = force_reg (mode, src1);

-   /* If optimizing, copy to regs to improve CSE */
-   if (optimize && ! no_new_pseudos)
-     {
-       if (GET_CODE (dst) == MEM)
- 	dst = gen_reg_rtx (mode);
-       if (GET_CODE (src1) == MEM)
- 	src1 = force_reg (mode, src1);
-       if (GET_CODE (src2) == MEM)
- 	src2 = force_reg (mode, src2);
-     }
-
     src1 = operands[1] = src1;
     src2 = operands[2] = src2;
     return dst;

This should be reinstated and conditionalized as

    if (optimize && !optimize_size && !no_new_pseudos
        && !TARGET_READ_MODIFY)

and likewise in ix86_expand_unary_operator.

*************** ix86_expand_fp_absneg_operator (enum rtx
*** 8410,8416 ****
     matching_memory = false;
     if (MEM_P (dst))
       {
!       if (rtx_equal_p (dst, src) && (!optimize || no_new_pseudos))
   	matching_memory = true;
         else
   	dst = gen_reg_rtx (mode);
--- 8626,8632 ----
     matching_memory = false;
     if (MEM_P (dst))
       {
!       if (rtx_equal_p (dst, src))
   	matching_memory = true;
         else
   	dst = gen_reg_rtx (mode);

This should become something like

    if (!rtx_equal_p (dst, src)
        || (optimize && !optimize_size && !no_new_pseudos
            && !TARGET_READ_MODIFY))
      dst = gen_reg_rtx (mode);
    else
      matching_memory = true;

Reversing the if allows to write the same test as above.

Does this make sense?

Paolo