| Message ID | 20110725223319.petrcv01csccs44s-nzlynne@webmail.spamcop.net |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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.
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
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
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