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 ^_^ ^_^ -- Dominik Vogt IBM Germany