| Message ID | 5a4bf204-9919-7930-affe-f854f6ad9171@redhat.com |
|---|---|
| State | New |
| Headers | show |
> So here's that cleanup. The diffs are larger than one might expect > because of the reindentation that needs to happen. So I've included a > -b diff variant which shows how little actually changed here. I'm wondering if it isn't counter-productive. The ??? comment is explicit about where the problem comes from: STACK_POINTER_OFFSET used to be defined only when needed, now it's always defined. So I think that we should try to restore the initial state, this will very likely generate fewer alignment operations, for example: #if defined (STACK_DYNAMIC_OFFSET) if (1) #else if (STACK_POINTER_OFFSET) #endif { must_align = true; extra_align = BITS_PER_UNIT; }
On 05/20/2016 03:44 PM, Eric Botcazou wrote: >> So here's that cleanup. The diffs are larger than one might expect >> because of the reindentation that needs to happen. So I've included a >> -b diff variant which shows how little actually changed here. > > I'm wondering if it isn't counter-productive. The ??? comment is explicit > about where the problem comes from: STACK_POINTER_OFFSET used to be defined > only when needed, now it's always defined. > > So I think that we should try to restore the initial state, this will very > likely generate fewer alignment operations, for example: I pondered that as a direction, but was scared off by the overall fragility of this code when I looked back through the old BZs. I figured cleanup preserving existing behavior was the first step. We can go the other way if you prefer. It just makes reasoning about how this code is supposed to work harder. jeff
> I pondered that as a direction, but was scared off by the overall > fragility of this code when I looked back through the old BZs. I > figured cleanup preserving existing behavior was the first step. Setting aside the flag_split_stack stuff, the must_align logic is clear enough and quite localized. > We can go the other way if you prefer. It just makes reasoning about > how this code is supposed to work harder. On second thoughts, there might be a real issue with STACK_DYNAMIC_OFFSET that is papered over by the STACK_POINTER_OFFSET glitch. The big comment explains why, if STACK_DYNAMIC_OFFSET is defined, we need to preventively align, and that it's defined if ACCUMULATE_OUTGOING_ARGS is activated. That's not true for STACK_DYNAMIC_OFFSET overall, but that's indeed true in function.c where STACK_DYNAMIC_OFFSET is forcibly defined. Given that ACCUMULATE_OUTGOING_ARGS is activated by default on most mainstream platforms, it probably makes sense to always preventively align. From there on, I think that what would be left to discuss is the amount of alignment we can start from (extra_align in the current code); maybe BITS_PER_UNIT is also the only sensible value in the end. So I think that your patch is OK if you rescue the main comment, for example: /* We will need to ensure that the address we return is aligned to REQUIRED_ALIGN. At this point in the compilation, we don't always know the final value of the STACK_DYNAMIC_OFFSET used in function.c (it might depend on the size of the outgoing parameter lists, for example), so we must preventively align the value. We leave space in SIZE for the hole that might result from the alignment operation. */
On Fri, May 20, 2016 at 03:23:49PM -0600, Jeff Law wrote: > On 05/19/2016 05:11 PM, Jeff Law wrote: > [ ... ] > >This is a bit of a mess and I think the code > >needs some TLC before we start hacking it up further. > > > >Let's start with clean up of dead code: > > > > /* We will need to ensure that the address we return is aligned to > > REQUIRED_ALIGN. If STACK_DYNAMIC_OFFSET is defined, we don't > > always know its final value at this point in the compilation (it > > might depend on the size of the outgoing parameter lists, for > > example), so we must align the value to be returned in that case. > > (Note that STACK_DYNAMIC_OFFSET will have a default nonzero value if > > STACK_POINTER_OFFSET or ACCUMULATE_OUTGOING_ARGS are defined). > > We must also do an alignment operation on the returned value if > > the stack pointer alignment is less strict than REQUIRED_ALIGN. > > > > If we have to align, we must leave space in SIZE for the hole > > that might result from the alignment operation. */ > > > > must_align = (crtl->preferred_stack_boundary < required_align); > > if (must_align) > > { > > if (required_align > PREFERRED_STACK_BOUNDARY) > > extra_align = PREFERRED_STACK_BOUNDARY; > > else if (required_align > STACK_BOUNDARY) > > extra_align = STACK_BOUNDARY; > > else > > extra_align = BITS_PER_UNIT; > > } > > > > /* ??? STACK_POINTER_OFFSET is always defined now. */ > >#if defined (STACK_DYNAMIC_OFFSET) || defined (STACK_POINTER_OFFSET) > > must_align = true; > > extra_align = BITS_PER_UNIT; > >#endif > > > >If we look at defaults.h, it always defines STACK_POINTER_OFFSET. So > >all the code above I think collapses to: > > > > must_align = true; > > extra_align = BITS_PER_UNIT > > > >And the only other assignment to must_align assigns it the value "true". > > There are two conditionals on must_align that looks like > > > >if (must_align) > > { > > CODE; > > } > > > >We should remove the conditional and pull CODE out an indentation level. > > And remove all remnants of must_align. > > > >I don't think that changes your patch in any way. Hopefully it makes > >the whole function somewhat easier to grok. > > > >Thoughts? > So here's that cleanup. The diffs are larger than one might expect > because of the reindentation that needs to happen. So I've included > a -b diff variant which shows how little actually changed here. > > This should have no impact on any target. > > Bootstrapped and regression tested on x86_64 linux. Ok for the trunk? The first part of the change (removing the dead code) is fine. However, I suggest to leave "must_align" in the code for now because I have another patch in the queue that assigns a calculated value to must_align. For that I'd have to revert this part of your patch, so I think it's not worth the effort to remove it in the first place. See https://gcc.gnu.org/ml/gcc-patches/2016-05/msg00445.html Ciao Dominik ^_^ ^_^
On Fri, May 20, 2016 at 03:23:49PM -0600, Jeff Law wrote: > On 05/19/2016 05:11 PM, Jeff Law wrote: > [ ... ] > >This is a bit of a mess and I think the code > >needs some TLC before we start hacking it up further. > > > >Let's start with clean up of dead code: > > > > /* We will need to ensure that the address we return is aligned to > > REQUIRED_ALIGN. If STACK_DYNAMIC_OFFSET is defined, we don't > > always know its final value at this point in the compilation (it > > might depend on the size of the outgoing parameter lists, for > > example), so we must align the value to be returned in that case. > > (Note that STACK_DYNAMIC_OFFSET will have a default nonzero value if > > STACK_POINTER_OFFSET or ACCUMULATE_OUTGOING_ARGS are defined). > > We must also do an alignment operation on the returned value if > > the stack pointer alignment is less strict than REQUIRED_ALIGN. > > > > If we have to align, we must leave space in SIZE for the hole > > that might result from the alignment operation. */ > > > > must_align = (crtl->preferred_stack_boundary < required_align); > > if (must_align) > > { > > if (required_align > PREFERRED_STACK_BOUNDARY) > > extra_align = PREFERRED_STACK_BOUNDARY; > > else if (required_align > STACK_BOUNDARY) > > extra_align = STACK_BOUNDARY; > > else > > extra_align = BITS_PER_UNIT; > > } > > > > /* ??? STACK_POINTER_OFFSET is always defined now. */ > >#if defined (STACK_DYNAMIC_OFFSET) || defined (STACK_POINTER_OFFSET) > > must_align = true; > > extra_align = BITS_PER_UNIT; > >#endif > > > >If we look at defaults.h, it always defines STACK_POINTER_OFFSET. So > >all the code above I think collapses to: > > > > must_align = true; > > extra_align = BITS_PER_UNIT > > > >And the only other assignment to must_align assigns it the value "true". > > There are two conditionals on must_align that looks like > > > >if (must_align) > > { > > CODE; > > } > > > >We should remove the conditional and pull CODE out an indentation level. > > And remove all remnants of must_align. > > > >I don't think that changes your patch in any way. Hopefully it makes > >the whole function somewhat easier to grok. > > > >Thoughts? > So here's that cleanup. The diffs are larger than one might expect > because of the reindentation that needs to happen. So I've included > a -b diff variant which shows how little actually changed here. > > This should have no impact on any target. > > Bootstrapped and regression tested on x86_64 linux. Ok for the trunk? I've rebased my patch on yours and also removed EXTRA_ALIGN which is also constant. I'll send new versions of both patches later. Ciao Dominik ^_^ ^_^
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index b8fb96c..257e98b 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,8 @@ +2016-05-20 Jeff Law <law@redhat.com> + + * explow.c (allocate_dynamic_stack_space): Simplify + knowing that MUST_ALIGN was always true. + 2016-05-16 Ryan Burn <contact@rnburn.com> * Makefile.in (GTFILES): Add cilk.h and cilk-common.c. diff --git a/gcc/explow.c b/gcc/explow.c index e0ce201..51897e0 100644 --- a/gcc/explow.c +++ b/gcc/explow.c @@ -1175,7 +1175,6 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, rtx_code_label *final_label; rtx final_target, target; unsigned extra_align = 0; - bool must_align; /* If we're asking for zero bytes, it doesn't matter what we point to since we can't dereference it. But return a reasonable @@ -1245,49 +1244,18 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, if (crtl->preferred_stack_boundary < PREFERRED_STACK_BOUNDARY) crtl->preferred_stack_boundary = PREFERRED_STACK_BOUNDARY; - /* We will need to ensure that the address we return is aligned to - REQUIRED_ALIGN. If STACK_DYNAMIC_OFFSET is defined, we don't - always know its final value at this point in the compilation (it - might depend on the size of the outgoing parameter lists, for - example), so we must align the value to be returned in that case. - (Note that STACK_DYNAMIC_OFFSET will have a default nonzero value if - STACK_POINTER_OFFSET or ACCUMULATE_OUTGOING_ARGS are defined). - We must also do an alignment operation on the returned value if - the stack pointer alignment is less strict than REQUIRED_ALIGN. - - If we have to align, we must leave space in SIZE for the hole - that might result from the alignment operation. */ - - must_align = (crtl->preferred_stack_boundary < required_align); - if (must_align) - { - if (required_align > PREFERRED_STACK_BOUNDARY) - extra_align = PREFERRED_STACK_BOUNDARY; - else if (required_align > STACK_BOUNDARY) - extra_align = STACK_BOUNDARY; - else - extra_align = BITS_PER_UNIT; - } - - /* ??? STACK_POINTER_OFFSET is always defined now. */ -#if defined (STACK_DYNAMIC_OFFSET) || defined (STACK_POINTER_OFFSET) - must_align = true; extra_align = BITS_PER_UNIT; -#endif - if (must_align) - { - unsigned extra = (required_align - extra_align) / BITS_PER_UNIT; + unsigned extra = (required_align - extra_align) / BITS_PER_UNIT; - size = plus_constant (Pmode, size, extra); - size = force_operand (size, NULL_RTX); + size = plus_constant (Pmode, size, extra); + size = force_operand (size, NULL_RTX); - if (flag_stack_usage_info) - stack_usage_size += extra; + if (flag_stack_usage_info) + stack_usage_size += extra; - if (extra && size_align > extra_align) - size_align = extra_align; - } + if (extra && size_align > extra_align) + size_align = extra_align; /* Round the size to a multiple of the required stack alignment. Since the stack if presumed to be rounded before this allocation, @@ -1366,7 +1334,6 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, gen_int_mode (required_align / BITS_PER_UNIT - 1, Pmode), NULL_RTX, 1, OPTAB_LIB_WIDEN); - must_align = true; } func = init_one_libfunc ("__morestack_allocate_stack_space"); @@ -1478,24 +1445,21 @@ allocate_dynamic_stack_space (rtx size, unsigned size_align, target = final_target; } - if (must_align) - { - /* CEIL_DIV_EXPR needs to worry about the addition overflowing, - but we know it can't. So add ourselves and then do - TRUNC_DIV_EXPR. */ - target = expand_binop (Pmode, add_optab, target, - gen_int_mode (required_align / BITS_PER_UNIT - 1, - Pmode), - NULL_RTX, 1, OPTAB_LIB_WIDEN); - target = expand_divmod (0, TRUNC_DIV_EXPR, Pmode, target, - gen_int_mode (required_align / BITS_PER_UNIT, - Pmode), - NULL_RTX, 1); - target = expand_mult (Pmode, target, - gen_int_mode (required_align / BITS_PER_UNIT, - Pmode), - NULL_RTX, 1); - } + /* CEIL_DIV_EXPR needs to worry about the addition overflowing, + but we know it can't. So add ourselves and then do + TRUNC_DIV_EXPR. */ + target = expand_binop (Pmode, add_optab, target, + gen_int_mode (required_align / BITS_PER_UNIT - 1, + Pmode), + NULL_RTX, 1, OPTAB_LIB_WIDEN); + target = expand_divmod (0, TRUNC_DIV_EXPR, Pmode, target, + gen_int_mode (required_align / BITS_PER_UNIT, + Pmode), + NULL_RTX, 1); + target = expand_mult (Pmode, target, + gen_int_mode (required_align / BITS_PER_UNIT, + Pmode), + NULL_RTX, 1); /* Now that we've committed to a return value, mark its alignment. */ mark_reg_pointer (target, required_align);