Re: GCC build failed for native with your patch on 2002-03-06T23:16:26Z.

["Ulrich Weigand" <Ulrich dot Weigand at de dot ibm dot com>]

Hello Richard,

this regression on PowerPC:

>The new failures are:
>powerpc-eabisim gcc.sum gcc.c-torture/execute/930818-1.c

is caused by the patch to unshare all rtl after reload.

However, it would appear that this is due to another bug that was
exposed by this change.  What happens is that regrename_optimize
produces an incorrect insn.

Specifically, this insn:

(insn 20 15 50 (parallel[
            (set (reg:CC 68 cr0 [123])
                (compare:CC (eq:SI (reg:CCFP 68 cr0 [122])
                        (const_int 0 [0x0]))
                    (const_int 0 [0x0])))
            (set (reg/v:SI 3 r3 [116])
                (eq:SI (reg:CCFP 68 cr0 [122])
                    (const_int 0 [0x0])))
        ] ) 388 {*rs6000.md:10567} (insn_list 15 (nil))
    (nil))

used to be transformed into:

(insn 20 15 50 (parallel[
            (set (reg:CC 68 cr0 [123])
                (compare:CC (eq:SI (reg:CCFP 69 cr1 [122])
                        (const_int 0 [0x0]))
                    (const_int 0 [0x0])))
            (set (reg/v:SI 3 r3 [116])
                (eq:SI (reg:CCFP 69 cr1 [122])
                    (const_int 0 [0x0])))
        ] ) 388 {*rs6000.md:10567} (insn_list 15 (nil))
    (expr_list:REG_DEAD (reg:CCFP 69 cr1 [122])
        (nil)))

but is now transformed into:

(insn 20 15 50 (parallel[
            (set (reg:CC 68 cr0 [123])
                (compare:CC (eq:SI (reg:CCFP 69 cr1 [122])
                        (const_int 0 [0x0]))
                    (const_int 0 [0x0])))
            (set (reg/v:SI 3 r3 [116])
                (eq:SI (reg:CCFP 68 cr0 [122])     <<=== here's the bug
                    (const_int 0 [0x0])))
        ] ) 388 {*rs6000.md:10567} (insn_list 15 (nil))
    (expr_list:REG_DEAD (reg:CCFP 69 cr1 [122])
        (nil)))

Note that the second occurrence of reg 68 is not replaced.
The insn pattern for this instruction is

  [(set (match_operand:CC 0 "cc_reg_operand" "=x,?y")
        (compare:CC (match_operator:SI 1 "scc_comparison_operator"
                                       [(match_operand 2 "cc_reg_operand" "y,y")
                                        (const_int 0)])
                    (const_int 0)))
   (set (match_operand:SI 3 "gpc_reg_operand" "=r,r")
        (match_op_dup 1 [(match_dup 2) (const_int 0)]))]

so the forgotten replacement happens inside a match_dup.

Without the rtl unsharing patch, these two locations were in fact
shared rtl, so that the bug did not show up!


When looking at the source (regrename.c: build_def_use) it would
appear that match_dup's are supposed to be handled correctly:

          /* Step 3: Append to chains for reads inside operands.  */
          for (i = 0; i < n_ops + recog_data.n_dups; i++)
            {
              int opn = i < n_ops ? i : recog_data.dup_num[i - n_ops];
              rtx *loc = (i < n_ops
                          ? recog_data.operand_loc[opn]
                          : recog_data.dup_loc[i - n_ops]);
              enum reg_class class = recog_op_alt[opn][alt].class;
              enum op_type type = recog_data.operand_type[opn];

              /* Don't scan match_operand here, since we've no reg class
                 information to pass down.  Any operands that we could
                 substitute in will be represented elsewhere.  */
              if (recog_data.constraints[opn][0] == '\0')
                continue;

              if (recog_op_alt[opn][alt].is_address)
                scan_rtx_address (insn, loc, class, mark_read, VOIDmode);
              else
                scan_rtx (insn, loc, class, mark_read, type, 0);
            }

However, this relies on recog_data correctly containing all dup locations,
which is not the case:  while we have in insn-extract.c:

    case 388:  /* *rs6000.md:10567 */
      ro[0] = *(ro_loc[0] = &XEXP (XVECEXP (pat, 0, 0), 0));
      ro[1] = *(ro_loc[1] = &XEXP (XEXP (XVECEXP (pat, 0, 0), 1), 0));
      ro[2] = *(ro_loc[2] = &XEXP (XEXP (XEXP (XVECEXP (pat, 0, 0), 1), 0), 0));
      ro[3] = *(ro_loc[3] = &XEXP (XVECEXP (pat, 0, 1), 0));
      recog_data.dup_loc[0] = &XEXP (XVECEXP (pat, 0, 1), 1);
      recog_data.dup_num[0] = 1;
      recog_data.dup_loc[1] = &XEXP (XEXP (XVECEXP (pat, 0, 1), 1), 0);
      recog_data.dup_num[1] = 2;
      break;

the insn_data struct in insn-output.c is defined as:

  {
    "*rs6000.md:10567",
    (const PTR) output_388,
    0,
    &operand_data[848],
    4,
    1,                <==== n_dups
    2,
    2
  },

Note that n_dups is 1, and not 2.  The reason for this in turn is that the
match_dup is *inside* a match_op_dup in the insn pattern, and while the
corresponding code in genextract.c does recurse into a match_op_dup, the
genoutput.c code does not.

The following patch fixes this problem, and thus the test case.  Do you
think this is the correct fix, and would it be OK for branch and trunk
(after bootstrap / regtest)?

Index: gcc/genoutput.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/genoutput.c,v
retrieving revision 1.68
diff -c -p -r1.68 genoutput.c
*** genoutput.c     2001/12/02 00:04:19  1.68
--- genoutput.c     2002/03/07 17:51:12
*************** scan_operands (d, part, this_address_p,
*** 527,536 ****
        return;

      case MATCH_DUP:
-     case MATCH_OP_DUP:
      case MATCH_PAR_DUP:
        ++num_dups;
        return;

      case ADDRESS:
        scan_operands (d, XEXP (part, 0), 1, 0);
--- 527,539 ----
        return;

      case MATCH_DUP:
      case MATCH_PAR_DUP:
        ++num_dups;
        return;
+
+     case MATCH_OP_DUP:
+       ++num_dups;
+       break;

      case ADDRESS:
        scan_operands (d, XEXP (part, 0), 1, 0);


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand
  Linux for S/390 Design & Development
  IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
  Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com