Re: [PATCH] Check for sp push/pop insns in reg_set_p (PR target/79430)

[Jeff Law <law@redhat.com>]

On 04/27/2017 01:32 AM, Jakub Jelinek wrote:
> Hi!
> 
> As mentioned in the PR and can be seen on the testcase (too large for
> testsuite, with lots of delta reduction I got 48KB *.f90 file still using
> a dozen of modules), we miscompile it because we have mem(sp+64) memory
> (what %st is loaded from) and are checking whether it is safe to move
> earlier in the insn stream, and modified_between_p tells us it is, except
> there is a stack pop instruction (i.e. sp autoinc).
> And sp autoinc is apparently special in GCC:
>        /* There are no REG_INC notes for SP.  */
Right.  It's been the source of numerous problems through the years. 
One could argue that we should just bite the bullet and add them.  The 
cost can't be that high and it'd avoid these kinds of problems in the 
future and allow for some code cleanups as well.

We could probably scan the IL at the end of auto-inc-dec.c to add the 
missing notes.

I thought I saw a comment once which indicates the rationale behind not 
including REG_INC notes for pushes/pops, but I can't find it anymore.

> 
> The following patch handles that, plus then undoes that in ix86_agi_dependent
> where from what I understood we want the previous behavior - push, pop and
> call modifications of SP don't cause AGI stalls for addresses that have
> SP base (SP can't appear as index).
> 
> Not really sure about the == stack_pointer_rtx vs.
> REG_P () && REGNO () == STACK_POINTER_REGNUM, there is lots of code that
> just uses pointer comparisons and others that check REGNO, as an example
> of the former e.g. push/pop_operand.  So, is SP always shared, or can there
> be other REGs with SP regno?
SP is supposed to be shared, you should be able to compare against 
stack_pointer_rtx.


> 
> Other than the ix86_agi_dependent which in my stats was the single case
> that hit this difference, I've seen it making a difference e.g. in ifcvt
> decisions, but at least the cases I've debugged didn't end up in any code
> generation changes.  E.g. both x86_64 and i686 libstdc++.so.6 and
> libgo.so.11 as the two largest shared libraries built during bootstrap
> are identical without/with this patch (objdump -dr is identical that is).
> While without the config/i386/i386.c changes there were tons of differences.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2017-04-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/79430
> 	* rtlanal.c (reg_set_p): If reg is a stack_pointer_rtx, also
> 	check for stack push/pop autoinc.
> 	* config/i386/i386.c (ix86_agi_dependent): Return false
> 	if the only reason why modified_in_p returned true is that
> 	addr is SP based and set_insn is a push or pop.
THe rtlanal.c changes are fine by me.  Uros should chime in on the x86 
specific bits.

Jeff