On 11/22/2017 10:46 AM, Richard Sandiford wrote:
> One of the effects of:
>
> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02066.html
>
> is that we now emit:
>
> (set (hard-frame-pointer) (stack-pointer))
>
> instead of the previous non-canonical:
>
> (set (hard-frame-pointer) (plus (stack-pointer) (const_int 0)))
>
> However, recent changes to aarch64_expand_prologue mean that we
> can emit a frame record even if !frame_pointer_needed. In that case,
> the frame pointer is initialised normally, but all references generated
> by GCC continue to use the stack pointer.
>
> The combination of these two things triggered a regression in
> gcc.c-torture/execute/960419-2.c. The problem is that alias.c
> fundemantally requires GCC to be consistent about which register
> it uses:
>
> 2. stack_pointer_rtx, frame_pointer_rtx, hard_frame_pointer_rtx
> (if distinct from frame_pointer_rtx) and arg_pointer_rtx.
> Each of these rtxes has a separate ADDRESS associated with it,
> each with a negative id.
>
> GCC is (and is required to be) precise in which register it
> chooses to access a particular region of stack. We can therefore
> assume that accesses based on one of these rtxes do not alias
> accesses based on another of these rtxes.
Yea. I remember acking this code for jfc circa 1998.
>
> But in the test case cselib saw the move above and recorded the two
> registers as being equivalent. We therefore replaced some (but not all)
> references to the stack pointer with references to the frame pointer.
> MEMs that were still based on the stack pointer would then not alias
> MEMs that became based on the frame pointer.
>
> cselib.c has code to try to prevent this:
>
> for (; p; p = p->next)
> {
> /* Return these right away to avoid returning stack pointer based
> expressions for frame pointer and vice versa, which is something
> that would confuse DSE. See the comment in cselib_expand_value_rtx_1
> for more details. */
> if (REG_P (p->loc)
> && (REGNO (p->loc) == STACK_POINTER_REGNUM
> || REGNO (p->loc) == FRAME_POINTER_REGNUM
> || REGNO (p->loc) == HARD_FRAME_POINTER_REGNUM
> || REGNO (p->loc) == cfa_base_preserved_regno))
> return p->loc;
>
> which refers to:
>
> /* The only thing that we are not willing to do (this
> is requirement of dse and if others potential uses
> need this function we should add a parm to control
> it) is that we will not substitute the
> STACK_POINTER_REGNUM, FRAME_POINTER or the
> HARD_FRAME_POINTER.
>
> These expansions confuses the code that notices that
> stores into the frame go dead at the end of the
> function and that the frame is not effected by calls
> to subroutines. If you allow the
> STACK_POINTER_REGNUM substitution, then dse will
> think that parameter pushing also goes dead which is
> wrong. If you allow the FRAME_POINTER or the
> HARD_FRAME_POINTER then you lose the opportunity to
> make the frame assumptions. */
>
> But that doesn't work if the two registers are equal, and thus
> in the same value chain. It becomes pot luck which one is chosen.
Right. I can't recall these bits in cselib.
>
> I think it'd be better not to enter an equivalence for the set of
> the frame pointer at all. That should deal with the same correctness
> concern as the code above, but I think we still want the existing checks
> for optimisation reasons. It seems better to check for pointer equality
> instead of register number equality though, since we don't want to change
> the behaviour when the frame pointer register is not being used as a frame
> pointer.
You have to be real careful here. regcprop can create copies of the
special registers -- I ran into it making a copy of stack_pointer_rtx
earlier this year. It special cases the stack pointer now with a comment
"It's unclear if we need to do the same for other special registers".
>
> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linux-gnu.
> Does it look like the right approach? OK to install if so?
>
> Thanks,
> Richard
>
>
> 2017-11-22 Richard Sandiford <richard.sandif...@linaro.org>
>
> gcc/
> * cselib.c (expand_loc, cselib_expand_value_rtx_1): Change
> justification for checking for the stack and hard frame pointers.
> Check them by pointer rather than register number.
> (cselib_record_set): Do nothing for sets of the frame pointer.
I think the big question here is the pointer equality vs register number
check. Prior to reload/lra we're probably safe with the former, after
we probably need the latter, likely conditionalized with
frame_pointer_needed or somesuch to avoid triggering when we just have a
value in the frame pointer register, but not an actual frame pointer.
Alternately, bring further sense to the special register handling in
regcprop and this patch probably becomes OK as-is.
jeff
