This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Fix PR middle-end/28690, indexed load/store performance + reload bug
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: bergner at vnet dot ibm dot com (Peter Bergner)
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Wed, 6 Dec 2006 22:27:57 +0100 (CET)
- Subject: 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:
> > 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
[(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 ] )
(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.
Dr. Ulrich Weigand
GNU Toolchain for Linux on System z and Cell BE