Re: RFC/A: Stop cselib.c recording sets of the frame pointer

[Jeff Law <law at redhat dot com>]

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.sandiford@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