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

[Uros Bizjak <ubizjak@gmail.com>]

On Thu, Apr 27, 2017 at 6:13 PM, Jeff Law <law@redhat.com> wrote:
> 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.

LGTM, with comparison to stack_pointer_rtx, as mentioned by Jeff above.

Thanks,
Uros.