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.
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.
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.
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.
Index: gcc/cselib.c
===================================================================
*** gcc/cselib.c 2017-11-21 17:57:59.245162437 +0000
--- gcc/cselib.c 2017-11-22 17:43:08.894805286 +0000
*************** expand_loc (struct elt_loc_list *p, stru
*** 1420,1433 ****
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;
/* Avoid infinite recursion trying to expand a reg into a
--- 1420,1435 ----
for (; p; p = p->next)
{
! /* Prefer to use references to invariant registers like the stack
! and hard frame pointers, since they are usually easier to analyze.
! E.g. alias.c can usually provide better disambiguation if given
! an address based on the stack pointer instead of one based on
! an arbitrary hard register that is set multiple times in the
! function. */
if (REG_P (p->loc)
! && (p->loc == stack_pointer_rtx
! || p->loc == frame_pointer_rtx
! || p->loc == hard_frame_pointer_rtx
|| REGNO (p->loc) == cfa_base_preserved_regno))
return p->loc;
/* Avoid infinite recursion trying to expand a reg into a
*************** cselib_expand_value_rtx_1 (rtx orig, str
*** 1594,1618 ****
rtx result;
unsigned regno = REGNO (orig);
! /* 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. */
! if (regno == STACK_POINTER_REGNUM
! || regno == FRAME_POINTER_REGNUM
! || regno == HARD_FRAME_POINTER_REGNUM
|| regno == cfa_base_preserved_regno)
return orig;
--- 1596,1610 ----
rtx result;
unsigned regno = REGNO (orig);
! /* Prefer to keep references to invariant registers like
! the stack and hard frame pointer. Replacing them with
! references to non-invariant registers would pessimise
! the information provided by alias.c, at least if the
! non-invariant register is set more than once in the
! function. */
! if (orig == stack_pointer_rtx
! || orig == frame_pointer_rtx
! || orig == hard_frame_pointer_rtx
|| regno == cfa_base_preserved_regno)
return orig;
*************** cselib_record_set (rtx dest, cselib_val
*** 2368,2373 ****
--- 2360,2374 ----
if (REG_P (dest))
{
+ /* Do not record equivalences for the frame pointer, since that is
+ ultimately set from the stack pointer. We need to maintain
+ the invariant (relied on by alias.c) that references to a given
+ region of the stack consistently use the frame pointer or
+ consistently use the stack pointer; we cannot mix the two. */
+ if (dest == hard_frame_pointer_rtx
+ || dest == frame_pointer_rtx)
+ return;
+
unsigned int dreg = REGNO (dest);
if (dreg < FIRST_PSEUDO_REGISTER)
{
