On Mon, Dec 15, 2014 at 04:51:14PM +0100, Paolo Bonzini wrote:
> Random questions:
>
> 1) did you check that it never triggers on e.g. an x86 bootstrap, and
> that it doesn't trigger too often on PPC64?
I have checked on my largish connection of tests for the carry insns
on PowerPC, and only two (related) transforms are disabled, and they
aren't too important anyway. Well, and the bad transforms are disabled,
only just two of-em but much more frequent (long long x; x--;).
I haven't checked on x86, but it's a bugfix: don't do things that blow up!
It is amazing to me that it didn't show up before. One theory is that
instructions that set the condition code as well as a GPR will never
combine with a later insn to two insns, always to just one. But nothing
made this explicit so AFAICS it is just an accident that it worked before.
I'll do an instrumented x86 bootstrap.
[ That testcase, -m32:
long long addSH(long long a, unsigned long b)
{
return a + ((unsigned long long)b << 32);
}
results in addic 4,4,0 ; addze 3,5 while it could just be add 3,5 ]
> 2) if it triggers rarely, should combine just try and give a new UID to
> i1?
That should in principle works, sure. Seems too dangerous for stage3
though. combine cannot create UIDs as things stand now.
> What makes that hard? Or should it just skip the SET_INSN_DELETED
> on i1 unless it is added to the instruction stream?
I have tried that first, but it blew up in other ways. I didn't look
too closely, sorry.
It really is quite dangerous to allow combining just two original insns to
two other insns; there are arguments why it cannot loop in this case (it
always moves a parallel down, for example) but that is quite fragile, and
also not documented anywhere. It also _did_ loop with some of my (perhaps
broken) patch attempts. So I opted for the heavy hammer approach.
> That said, if (1) is true, this looks like this fix is good enough for 5
> and open branches.
>
> Thanks David for pointing this out to me.
I need to remember to CC: people, sorry :-/
Thanks for looking,
Segher
