[RFC] Add middle end hook for stack red zone size
diff mbox

Message ID 20110725223319.petrcv01csccs44s-nzlynne@webmail.spamcop.net
State New
Headers show

Commit Message

Joern Rennecke July 26, 2011, 2:33 a.m. UTC
Quoting Jiangning Liu <jiangning.liu@arm.com>:

> Hi,
>
> One month ago, I sent out this RFC to *gcc-patches* mail list, but I  
>  didn't receive any response yet. So I'm forwarding this mail to   
> *gcc* mail list. Can anybody here really give feedback to me?

Well, I couldn't approve any patch, but I can point out some issues
with your patch.

First, it's missing a ChangeLog, and you don't state how you have tested it.
And regarding the code in sched_analyze_1, I think you'll get false
positives with alloca, and false negatives when registers are involved
to compute offsets or to restore the stack pointer from.

FWIW, I think generally blunt scheduling barriers should be avoided, and
instead the dependencies made visible to the scheduler.
E.g., I've been working with another architecture with a redzone, where at
-fno-omit-frame-pointer, the prologue can put pretend_args into the redzone,
then after stack adjustment and frame allocation, these arguments are
accessed via the frame pointer.

With the attached patchlet, alias analysis works for this situation too, so
no blunt scheduling block is required.

Likewise, with stack adjustments, they should not affect scheduling in
general, but be considered to clobber the part of the frame that is
being exposed to interrupt writes either before or after the adjustment.
At the moment, each port that wants to have such selective scheduling
blockages has to define a stack_adjust pattern with a memory clobber  
in a parallel, with a memref that shows the high water mark of possible
interrupt stack writes.
Prima facia it would seem convenient if you only had to tell the middle-end
about the redzone size, and it could figure out the implicit clobbers when
the stack is changed.  However, when a big stack adjustment is being made,
or the stack is realigned, or restored from the frame pointer / another
register where it was saved due to realignment, the adjustment is not
so obvious.  I'm not sure if you can actually create an robust interface
that's simpler to use than putting the right memory clobber in the stack
adjust pattern.  Note also that the redzone size can vary from function
to function depending on ABI-altering attributes, in particular for interrupt
functions, which can also have different incoming and outgoing redzone
sizes.  Plus, you can have an NMI / reset handler which can use the stack
like an ordinary address register.
2009-09-23  Joern Rennecke <joern.rennecke@embecosm.com>

	* alias.c (base_alias_check): Allow for aliases between stack-
	and frame-pointer referenced memory.

Comments

Jiangning Liu Aug. 1, 2011, 3:44 a.m. UTC | #1
Joern,

Thanks for your valuable feedback! This is only a RFC, and I will send out formal patch along with ChangLog later on. 

Basically, my patch is only to add new dependence in scheduler, and it only blocks some instruction movements, so it is NO RISK to compiler correctness. For whatever stack pointer changes you gave in different scenarios, the current code base should already work. My patch intends neither to replace old dependences, nor maximize the scheduler capability due to the existence of red zone in stack. It is only to block the memory access moving over stack pointer adjustment if distance is beyond red zone size, which is an OS requirement due to interruption existence. 

Stack adjustment in epilogue is a very general usage in stack frame. It's quite necessary to solve the general problem in middle-end rather than in back-end. Also, that old patch you attached is to solve the data dependence between two memory accesses, but stack pointer doesn't really have data dependence with memory access without using stack pointer, so they have different stories. Alternative solution of without adding blunt scheduling barrier is we insert an independent pass before scheduler to create RTL barrier by using the same interface stack_red_zone_size, but it would really be an over-design, if we add a new pass only for this *small* functionality.

In my patch, *abs* of offset is being used, so you are right that it's possible to get false positive to be too conservative, but there won't exist false negative, because my code would only add new dependences. 

Since the compilation is based on function, it would be OK if red zone size varies due to different ABI. Could you please tell me exactly on what system being supported by GCC red zone size can be different for incoming and outgoing? And also how scheduler guarantee the correctness in current code base? Anyway, I don't think my patch will break the original solution.

Thanks,
-Jiangning

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org]
> On Behalf Of Joern Rennecke
> Sent: Tuesday, July 26, 2011 10:33 AM
> To: Jiangning Liu
> Cc: gcc@gcc.gnu.org; gcc-patches@gcc.gnu.org; vmakarov@redhat.com;
> dje.gcc@gmail.com; Richard Henderson; Ramana Radhakrishnan; 'Ramana
> Radhakrishnan'
> Subject: RE: [RFC] Add middle end hook for stack red zone size
> 
> Quoting Jiangning Liu <jiangning.liu@arm.com>:
> 
> > Hi,
> >
> > One month ago, I sent out this RFC to *gcc-patches* mail list, but I
> >  didn't receive any response yet. So I'm forwarding this mail to
> > *gcc* mail list. Can anybody here really give feedback to me?
> 
> Well, I couldn't approve any patch, but I can point out some issues with your patch.
> 
> First, it's missing a ChangeLog, and you don't state how you have tested it.
> And regarding the code in sched_analyze_1, I think you'll get false positives with
> alloca, and false negatives when registers are involved to compute offsets or to
> restore the stack pointer from.
> 
> FWIW, I think generally blunt scheduling barriers should be avoided, and instead
> the dependencies made visible to the scheduler.
> E.g., I've been working with another architecture with a redzone, where at -fno-
> omit-frame-pointer, the prologue can put pretend_args into the redzone, then after
> stack adjustment and frame allocation, these arguments are accessed via the frame
> pointer.
> 
> With the attached patchlet, alias analysis works for this situation too, so no blunt
> scheduling block is required.
> 
> Likewise, with stack adjustments, they should not affect scheduling in general, but
> be considered to clobber the part of the frame that is being exposed to interrupt
> writes either before or after the adjustment.
> At the moment, each port that wants to have such selective scheduling blockages
> has to define a stack_adjust pattern with a memory clobber in a parallel, with a
> memref that shows the high water mark of possible interrupt stack writes.
> Prima facia it would seem convenient if you only had to tell the middle-end about
> the redzone size, and it could figure out the implicit clobbers when the stack is
> changed.  However, when a big stack adjustment is being made, or the stack is
> realigned, or restored from the frame pointer / another register where it was
> saved due to realignment, the adjustment is not so obvious.  I'm not sure if you can
> actually create an robust interface that's simpler to use than putting the right
> memory clobber in the stack adjust pattern.  Note also that the redzone size can
> vary from function to function depending on ABI-altering attributes, in particular
> for interrupt functions, which can also have different incoming and outgoing
> redzone sizes.  Plus, you can have an NMI / reset handler which can use the stack
> like an ordinary address register.
Jakub Jelinek Aug. 1, 2011, 9:11 a.m. UTC | #2
On Mon, Aug 01, 2011 at 11:44:04AM +0800, Jiangning Liu wrote:
> It's quite necessary to solve the general problem in middle-end rather than in back-end.

That's what we disagree on.  All back-ends but ARM are able to handle it
right, why can't ARM too?  The ABI rules for stack handling in the epilogues
are simply too diverse and complex to be handled easily in the scheduler.

	Jakub
Joseph Myers Aug. 1, 2011, 10:03 a.m. UTC | #3
On Mon, 1 Aug 2011, Jakub Jelinek wrote:

> On Mon, Aug 01, 2011 at 11:44:04AM +0800, Jiangning Liu wrote:
> > It's quite necessary to solve the general problem in middle-end rather than in back-end.
> 
> That's what we disagree on.  All back-ends but ARM are able to handle it
> right, why can't ARM too?  The ABI rules for stack handling in the epilogues
> are simply too diverse and complex to be handled easily in the scheduler.

Given that the long-standing open bugs relating to scheduling and stack 
adjustments (30282, 38644) include issues for Power that do not yet appear 
to have been fixed, even if other back ends are able to handle it right 
they don't seem to do so at present.
Jiangning Liu Aug. 1, 2011, 10:14 a.m. UTC | #4
The answer is ARM can. However, if you look into the bugs PR30282 and 
PR38644, PR44199, you may find in history, there are several different cases

in different ports reporting the similar failures, covering x86, PowerPC and

ARM. You are right, they were all fixed in back-ends in the past, but we
should 
fix the bug in a general way to make GCC infrastructure stronger, rather 
than fixing the problem target-by-target and case-by-case! If you further 
look into the back-end fixes in x86 and PowerPC, you may find they looks 
quite similar in back-ends. 

Thanks,
-Jiangning

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org]
> On Behalf Of Jakub Jelinek
> Sent: Monday, August 01, 2011 5:12 PM
> To: Jiangning Liu
> Cc: 'Joern Rennecke'; gcc@gcc.gnu.org; gcc-patches@gcc.gnu.org;
> vmakarov@redhat.com; dje.gcc@gmail.com; Richard Henderson; Ramana
> Radhakrishnan; 'Ramana Radhakrishnan'
> Subject: Re: [RFC] Add middle end hook for stack red zone size
> 
> On Mon, Aug 01, 2011 at 11:44:04AM +0800, Jiangning Liu wrote:
> > It's quite necessary to solve the general problem in middle-end rather
than in
> back-end.
> 
> That's what we disagree on.  All back-ends but ARM are able to handle it
> right, why can't ARM too?  The ABI rules for stack handling in the
epilogues
> are simply too diverse and complex to be handled easily in the scheduler.
> 
> 	Jakub
Richard Earnshaw Aug. 1, 2011, 10:16 a.m. UTC | #5
On 01/08/11 10:11, Jakub Jelinek wrote:
> On Mon, Aug 01, 2011 at 11:44:04AM +0800, Jiangning Liu wrote:
>> It's quite necessary to solve the general problem in middle-end rather than in back-end.
> 
> That's what we disagree on.  All back-ends but ARM are able to handle it
> right, why can't ARM too?  The ABI rules for stack handling in the epilogues
> are simply too diverse and complex to be handled easily in the scheduler.

Because the vast majority of back-ends (ie those without red zones)
shouldn't have to deal with this mess.  This is something the MI code
should be able to work out and deal with itself.  Then the compiler will
at least generate safe code, rather than randomly moving things about
and allowing potentially unsafe writes/reads from beyond the allocated
stack region.

We should build the compiler defensively, but then allow for more
aggressive optimizations to disable the defences when the back-end wants
to take on the responsibility.  Not the other way around.

R.
Jakub Jelinek Aug. 1, 2011, 10:30 a.m. UTC | #6
On Mon, Aug 01, 2011 at 06:14:27PM +0800, Jiangning Liu wrote:
> ARM. You are right, they were all fixed in back-ends in the past, but we
> should 
> fix the bug in a general way to make GCC infrastructure stronger, rather 
> than fixing the problem target-by-target and case-by-case! If you further 
> look into the back-end fixes in x86 and PowerPC, you may find they looks 
> quite similar in back-ends. 
> 

Red zone is only one difficulty, your patch is e.g. completely ignoring
existence of biased stack pointers (e.g. SPARC -m64 has them).
Some targets have stack growing in opposite direction, etc.
We have really a huge amount of very diverse ABIs and making the middle-end
grok what is an invalid stack access is difficult.

	Jakub
Jiangning Liu Aug. 2, 2011, 4:45 a.m. UTC | #7
Hi Jakub,

Appreciate for your valuable comments!

I think SPARC V9 ABI doesn't have red zone defined, right? So
stack_red_zone_size should be defined as zero by default, the scheduler
would block moving memory accesses across stack adjustment no matter what
the offset is. I don't see any risk here. Also, in my patch function *abs*
is being used to avoid the opposite stack direction issue as you mentioned.

Some people like you insist on the ABI diversity, and actually I agree with
you on this. But part of the ABI definition is general for all targets. The
point here is memory access beyond stack red zone should be avoided, which
is the general part of ABI that compiler should guarantee. For this general
part, middle end should take the responsibility.

Thanks,
-Jiangning

> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Monday, August 01, 2011 6:31 PM
> To: Jiangning Liu
> Cc: 'Joern Rennecke'; gcc@gcc.gnu.org; gcc-patches@gcc.gnu.org;
> vmakarov@redhat.com; dje.gcc@gmail.com; Richard Henderson; Ramana
> Radhakrishnan; 'Ramana Radhakrishnan'
> Subject: Re: [RFC] Add middle end hook for stack red zone size
> 
> On Mon, Aug 01, 2011 at 06:14:27PM +0800, Jiangning Liu wrote:
> > ARM. You are right, they were all fixed in back-ends in the past, but
> we
> > should
> > fix the bug in a general way to make GCC infrastructure stronger,
> rather
> > than fixing the problem target-by-target and case-by-case! If you
> further
> > look into the back-end fixes in x86 and PowerPC, you may find they
> looks
> > quite similar in back-ends.
> >
> 
> Red zone is only one difficulty, your patch is e.g. completely ignoring
> existence of biased stack pointers (e.g. SPARC -m64 has them).
> Some targets have stack growing in opposite direction, etc.
> We have really a huge amount of very diverse ABIs and making the
> middle-end
> grok what is an invalid stack access is difficult.
> 
> 	Jakub

Patch
diff mbox

Index: alias.c
===================================================================
--- alias.c	(revision 1646)
+++ alias.c	(revision 1647)
@@ -1751,6 +1751,17 @@  base_alias_check (rtx x, rtx y, enum mac
   if (GET_CODE (x_base) != ADDRESS && GET_CODE (y_base) != ADDRESS)
     return 0;
 
+  /* If both are stack references, one via the stack pointer, the other via
+     the frame pointer, there can be an alias.
+     E.g.: gcc.c-torture/execute/20041113-1.c -O3 -g  */
+  if (GET_CODE (x_base) == ADDRESS
+      && (XEXP (x_base, 0) == stack_pointer_rtx
+	  || XEXP (x_base, 0) == hard_frame_pointer_rtx)
+      && GET_CODE (y_base) == ADDRESS
+      && (XEXP (y_base, 0) == stack_pointer_rtx
+	  || XEXP (y_base, 0) == hard_frame_pointer_rtx))
+    return 1;
+
   /* If one address is a stack reference there can be no alias:
      stack references using different base registers do not alias,
      a stack reference can not alias a parameter, and a stack reference