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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix PR middle-end/28690, indexed load/store performance + reload bug


On Wed, Dec 06, 2006 at 03:10:05PM +0100, Ulrich Weigand wrote:
> I'm not sure it is necessary to spend a lot of effort trying to fix
> up a portion of the PARALLEL is we already know is unused.  I assume
> the problem occurs in the second half of this if:
[snip]
> where it tries to modify the existing single_set.

No, the problem occurs after we've returned from eliminate_regs_in_insn().
To reiterate, the insn before the call to eliminate_regs_in_insn() looks
like:

(insn 21 70 22 2 (parallel [
            (set (reg:SI 0 0 [128])
                (mem/s/j:SI (plus:SI (reg/f:SI 113 sfp)
                        (const_int 8 [0x8])) [0 S4 A32]))
            (set (reg/f:SI 9 9 [orig:124 D.966 ] [124])
                (plus:SI (reg/f:SI 113 sfp)
                    (const_int 8 [0x8])))
        ]) 374 {*movsi_update1} (nil)
    (expr_list:REG_UNUSED (reg:SI 0 0 [128])
        (nil)))

and after we return, we have:

(insn 21 70 22 2 (parallel [
            (set (reg:SI 0 0 [128])
                (mem/s/j:SI (plus:SI (reg/f:SI 113 sfp)
                        (const_int 8 [0x8])) [0 S4 A32]))
            (set (reg/f:SI 9 9 [orig:124 D.966 ] [124])
                (plus:SI (reg/f:SI 1 1)
                    (const_int 8 [0x8])))
        ]) 374 {*movsi_update1} (nil)
    (expr_list:REG_UNUSED (reg:SI 0 0 [128])
        (nil)))

Note: pseudo 113 is the virtual hard register for hard reg 1 (ie, the ppc stack
pointer reg).  The problematic code is located in find_reloads().  Specifically,
the snipit below.

  /* If this insn pattern contains any MATCH_DUP's, make sure that
     they will be substituted if the operands they match are substituted.
     Also do now any substitutions we already did on the operands.
  
     Don't do this if we aren't making replacements because we might be
     propagating things allocated by frame pointer elimination into places
     it doesn't expect.  */ 
  
  if (insn_code_number >= 0 && replace)
    for (i = insn_data[insn_code_number].n_dups - 1; i >= 0; i--)
      {
	int opno = recog_data.dup_num[i];
	*recog_data.dup_loc[i] = *recog_data.operand_loc[opno];  <- We overwrite the pattern here.
	dup_replacements (recog_data.dup_loc[i], recog_data.operand_loc[opno]);
      }

The second time through this loop, we have:

  (gdb) p opno
  $135 = 1
  (gdb) p i
  $136 = 0
  (gdb) pr *recog_data.operand_loc[opno]
  (reg/f:SI 113 sfp)
  (gdb) pr *recog_data.dup_loc[i]
  (reg/f:SI 1 1)

So we end up overwriting the good (reg/f:SI 1 1) with the bad non-eliminated
(reg/f:SI 113 sfp) because we've somehow determined they are MATCH_DUP's.
I don't know how they're determined to be dups, maybe it's due to some
inherent property of INSN's, or do we actually compare the patterns?
Anyway, it seems we either shouldn't consider them dups when we haven't
eliminated all regs in the insn, or we need to be careful to eliminate
all of the regs.  I don't pretend to know which solution is correct/best.


> Why can't we just always re-create a fresh assignment insn from scratch,
> as the first part of the if does?   Or else, fall through to the common
> part of eliminate_regs_in_insn that recurses through all parts of the
> insn.

I frankly don't understand all of the nuances of this code, so if you
think that's the correct way of handling things, then I'll take your
word for it.  Does this solution match up with the failure mode I
showed above?

Peter




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]