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