> 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
