| Message ID | VI1PR0802MB262179ECF073A4934A03F7FC838D0@VI1PR0802MB2621.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On 11/29/2016 04:10 AM, Wilco Dijkstra wrote: > GCC caches the whether a function is a leaf in crtl->is_leaf. Using this > in the backend is best as leaf_function_p may not work correctly (eg. while > emitting prolog or epilog code). There are many reads of crtl->is_leaf > before it is initialized. Many targets do in targetm.frame_pointer_required > (eg. arm, aarch64, i386, mips, sparc), which is called before register > allocation by ira_setup_eliminable_regset and sched_init. > > Additionally, SHRINK_WRAPPING_ENABLED calls targetm.have_simple_return, > which evaluates the condition of the simple_return instruction. On ARM > this results in a call to use_simple_return_p which requires crtl->is_leaf > to be set correctly. > > To fix this, initialize crtl->is_leaf in ira_setup_eliminable_regset and > early on in ira. A bootstrap did not find any uninitialized reads of > crtl->is_leaf on Thumb-2. A follow-up patch will remove incorrect uses > of leaf_function_p from the ARM backend. > > Bootstrap OK (verified all reads of is_leaf in ARM backend are now after > initialization), OK for commit? > > ChangeLog: > 2016-11-29 Wilco Dijkstra <wdijkstr@arm.com> > > * gcc/ira.c (ira_setup_eliminable_regset): Initialize crtl->is_leaf. > (ira): Move initialization of crtl->is_leaf earlier. OK. jeff
Jeff Law wrote: > On 11/29/2016 04:10 AM, Wilco Dijkstra wrote: > > GCC caches the whether a function is a leaf in crtl->is_leaf. Using this > > in the backend is best as leaf_function_p may not work correctly (eg. while > > emitting prolog or epilog code). I forgot to ask, would it be reasonable to add an assert to check we're not in a sequence in leaf_function_p? I guess this will trigger on several targets (leaf_function_p is used in several backends) but it's a real bug if crtl->is_leaf is true. Wilco
On 11/29/2016 11:39 AM, Wilco Dijkstra wrote: > Jeff Law wrote: >> On 11/29/2016 04:10 AM, Wilco Dijkstra wrote: >>> GCC caches the whether a function is a leaf in crtl->is_leaf. Using this >>> in the backend is best as leaf_function_p may not work correctly (eg. while >>> emitting prolog or epilog code). > > I forgot to ask, would it be reasonable to add an assert to check we're not in > a sequence in leaf_function_p? I guess this will trigger on several targets > (leaf_function_p is used in several backends) but it's a real bug if > crtl->is_leaf is true. Can it wait for the next stage1? I'd hate to start tripping the assert all over the place at this point in the release cycle. jeff
Jeff Law wrote: > On 11/29/2016 11:39 AM, Wilco Dijkstra wrote: > > I forgot to ask, would it be reasonable to add an assert to check we're not in > > a sequence in leaf_function_p? I guess this will trigger on several targets > > (leaf_function_p is used in several backends) but it's a real bug if > > crtl->is_leaf is true. > Can it wait for the next stage1? I'd hate to start tripping the assert > all over the place at this point in the release cycle. Yes I don't think it is urgent as the incorrect value returned would likely make a leaf function save/restore the return address unnecessarily. It starts to generate incorrect code on ARM if you remove the if (reload_completed) test in arm_get_frame_offsets (which should just be an optimization to avoid recomputing the frame layout repeatedly, not essential for correctness). Wilco
On 11/29/2016 12:54 PM, Wilco Dijkstra wrote: > Jeff Law wrote: >> On 11/29/2016 11:39 AM, Wilco Dijkstra wrote: >>> I forgot to ask, would it be reasonable to add an assert to check we're not in >>> a sequence in leaf_function_p? I guess this will trigger on several targets >>> (leaf_function_p is used in several backends) but it's a real bug if >>> crtl->is_leaf is true. >> Can it wait for the next stage1? I'd hate to start tripping the assert >> all over the place at this point in the release cycle. > > Yes I don't think it is urgent as the incorrect value returned would likely make a leaf > function save/restore the return address unnecessarily. It starts to generate incorrect > code on ARM if you remove the if (reload_completed) test in arm_get_frame_offsets > (which should just be an optimization to avoid recomputing the frame layout repeatedly, > not essential for correctness). Feel free to go forward with this now on the trunk. If there's fallout it'll need to be fixed obviously, but it won't affect the upcoming gcc-7 release. jeff
diff --git a/gcc/ira.c b/gcc/ira.c index d20ec99fae562930424023be93ac77bb376445ef..00d32c3732f67c19fac922ca80d1ed11e8fc645b 100644 --- a/gcc/ira.c +++ b/gcc/ira.c @@ -2266,6 +2266,10 @@ ira_setup_eliminable_regset (void) int i; static const struct {const int from, to; } eliminables[] = ELIMINABLE_REGS; + /* Setup is_leaf as frame_pointer_required may use it. This function + is called by sched_init before ira.f scheduling is enabled. */ + crtl->is_leaf = leaf_function_p (); + /* FIXME: If EXIT_IGNORE_STACK is set, we will not save and restore sp for alloca. So we can't eliminate the frame pointer in that case. At some point, we should improve this by emitting the @@ -5074,6 +5078,13 @@ ira (FILE *f) clear_bb_flags (); + /* Determine if the current function is a leaf before running IRA + since this can impact optimizations done by the prologue and + epilogue thus changing register elimination offsets. + Other target callbacks may use crtl->is_leaf too, including + SHRINK_WRAPPING_ENABLED, so initialize as early as possible. */ + crtl->is_leaf = leaf_function_p (); + /* Perform target specific PIC register initialization. */ targetm.init_pic_reg (); @@ -5159,11 +5170,6 @@ ira (FILE *f) if (warn_clobbered) generate_setjmp_warnings (); - /* Determine if the current function is a leaf before running IRA - since this can impact optimizations done by the prologue and - epilogue thus changing register elimination offsets. */ - crtl->is_leaf = leaf_function_p (); - if (resize_reg_info () && flag_ira_loop_pressure) ira_set_pseudo_classes (true, ira_dump_file);
GCC caches the whether a function is a leaf in crtl->is_leaf. Using this in the backend is best as leaf_function_p may not work correctly (eg. while emitting prolog or epilog code). There are many reads of crtl->is_leaf before it is initialized. Many targets do in targetm.frame_pointer_required (eg. arm, aarch64, i386, mips, sparc), which is called before register allocation by ira_setup_eliminable_regset and sched_init. Additionally, SHRINK_WRAPPING_ENABLED calls targetm.have_simple_return, which evaluates the condition of the simple_return instruction. On ARM this results in a call to use_simple_return_p which requires crtl->is_leaf to be set correctly. To fix this, initialize crtl->is_leaf in ira_setup_eliminable_regset and early on in ira. A bootstrap did not find any uninitialized reads of crtl->is_leaf on Thumb-2. A follow-up patch will remove incorrect uses of leaf_function_p from the ARM backend. Bootstrap OK (verified all reads of is_leaf in ARM backend are now after initialization), OK for commit? ChangeLog: 2016-11-29 Wilco Dijkstra <wdijkstr@arm.com> * gcc/ira.c (ira_setup_eliminable_regset): Initialize crtl->is_leaf. (ira): Move initialization of crtl->is_leaf earlier. --