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


Peter Bergner wrote:

> 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().

Sorry for being unclear.  I do understand that it is secondary effects
some time after eliminate_regs_in_insn returns an insn with a partially-
eliminated sfp register that cause the bad code generation.

What I was refering to as "the problem" meant the code *within* 
eliminate_regs_in_insn that actually causes that function to return
an insn that isn't fully processed.

So to get back to my original question: can you verify that it is in fact
the code in the second half of the "if" I quoted in the earlier message
that causes eliminate_regs_in_insn to replace just the one occurrance of
the eliminable sfp register and then immediately return?

> 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.

find_reloads is correct here, the bug is for eliminate_regs_in_insn to
not actually eliminate all instances of an eliminable register.

That the two occurrences are dups is an inherent property of the
{*movsi_update1} pattern:

(define_insn "*movsi_update1"
  [(set (match_operand:SI 3 "gpc_reg_operand" "=r,r")
        (mem:SI (plus:SI (match_operand:SI 1 "gpc_reg_operand" "0,0")
                         (match_operand:SI 2 "reg_or_short_operand" "r,I"))))
   (set (match_operand:SI 0 "gpc_reg_operand" "=b,b")
        (plus:SI (match_dup 1) (match_dup 2)))]

Note the (match_dup ...) subexpressions.

> > 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?

The solution would return an insn containing only the 
            (set (reg/f:SI 9 9 [orig:124 D.966 ] [124])
                (plus:SI (reg/f:SI 1 1)
                    (const_int 8 [0x8])))
part, which would subsequently cause an addi instruction to be
generated.  Since there isn't any match_dup any more, that failure
mode cannot apply any longer.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com


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