On Thu, Apr 27, 2017 at 6:13 PM, Jeff Law <l...@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 <ja...@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.
