> On 01/01/2014 06:23 AM, Jan Hubicka wrote:
> >           last_sp_adjust = 0;
> > +         /* We no longer adjust stack size.  Whoever adjusted it earlier
> > +            hopefully got the note right.  */
> > +         note = find_reg_note (insn, REG_ARGS_SIZE, NULL_RTX);
> > +         if (note)
> > +           remove_note (insn, note);
> >           continue;
> 
> This is a totally unfounded hope, by the way.  I'm certain that I tried this

Sorry to hear that - that assumption seemed right in the testcase I loked into.

> the first time around, with similarly poor results.
> 
> See the g++.dg/opt/pr52727.C test case that fails with this patch.  I can only
> assume you didn't examine i686 test results when you committed this?
> 
> The problem is that the last adjustment _did_ apply the correct note
> adjustment, for the stack at that time.  But neither moving the note nor
> dropping the note is correct.
> 
> Consider
> 
>                                       // args_size 4
>       add     $4, %esp                // args_size 0
>       push    %eax                    // no note; fpu, not call related
> 
> transformed by csa to
> 
>       mov     %eax, (%esp)
> 
> If we move the note from the add to the mov, as we did prior to your patch, we
> retain the essence of the pop, that we're no longer queueing arguments.  But 
> if
> we drop the note, as we do with your patch, then we still believe that we have
> 4 bytes of arguments on the stack.
> 
> You say a bootstrap with Go was the testcase that sent you in this direction?
> I'll have a look...

Yes, if you remove the patch you should get ICE during go x86_64 bootstrap with
generic tuning.

I tried to describe the situation in original mail and the PR - the sequence was
same as you describe here, but comming from two consecutive calls, so args_size
was 8, then reduced to 0 by add and then increased to 8 by push.

I see in the testcase the psh comes from splitter - that is something I did
not consider indded. I am sorry for that.
How things are supposed to work in this case? So perhaps we need scheduler to
understand and move around the ARGS_SIZE note?

Honza

Reply via email to