Re: [patch, mips, tree] align microMIPS functions to 16 bits with -Os

[Richard Sandiford <rdsandiford at googlemail dot com>]

Richard Biener <rguenther@suse.de> writes:
> On Thu, 5 Jun 2014, Sandra Loosemore wrote:
>
>> On 06/05/2014 03:50 PM, Richard Sandiford wrote:
>> > Sandra Loosemore <sandra@codesourcery.com> writes:
>> > > On 06/05/2014 01:39 AM, Richard Biener wrote:
>> > > > 
>> > > > [snip]
>> > > > 
>> > > > Ok, we definitely need to preserve that (documented) behavior.  I
>> > > > suppose
>> > > > it also sets DECL_USER_ALIGN.  -falign-functions is probably another
>> > > > setter of DECL_ALIGN here.
>> > > > 
>> > > > If we add a target hook that may adjust function alignment then it
>> > > > has to honor any user set alignment, then -falign-functions and
>> > > > then it may only increase alignment over the default FUNCTION_BOUNDARY.
>> > > > 
>> > > > The point to adjust alignment with the hook may still be output time,
>> > > > but as we figured it can't simply ignore DECL_ALIGN.
>> > > 
>> > > Hmmmm.  The MIPS tweak we want here is to decrease the alignment for
>> > > certain functions, so I suppose this means we'd have to go about it
>> > > backwards by making FUNCTION_BOUNDARY 16 rather than 32 (at least for
>> > > -Os) and then later increasing it back to 32 for all non-microMIPS
>> > > functions.
>> > > 
>> > > Richard S., WDYT?  Shall I hack up another patch that does it that way,
>> > > or do you think that's still the wrong way to go about it?
>> > 
>> > Well, it sounds like the idea is that FUNCTION_BOUNDARY should basically
>> > be ignored as a DECL_ALIGN.  The only cases where DECL_ALIGN should matter
>> > are those where the user has set the alignment via command-line options
>> > or attributes.  If so, I'd rather just leave it as it is and make the
>> > hook pick something smaller.
>> > 
>> > Just to be sure: I'd imagined this working by having varasm.c detect
>> > when DECL_ALIGN needs to be honoured and having a hook to call when
>> > DECL_ALIGN is irrelevant.  The default version would then assert
>> > that DECL_ALIGN is FUNCTION_BOUNDARY and would return that alignment.
>> > The MIPS hook would override it for the special microMIPS case,
>> > but would call into the default otherwise.
>> > 
>> > (Hmm, maybe for that level of detail I should just have written a patch.
>> > Sorry about that...)
>
> Well, the question is what you initialize DECL_ALIGN to initially and how
> you ensure that docs are followed for aligned attribute (you can't
> decrease function alignment by it).  Adjusting a pre-existing DECL_ALIGN
> to sth smaller at any point sounds dangerous.  So why not go with
> Sandras idea instead?  I'd transition FUNCTION_BOUNDARY to a target hook
> like
>
>   unsigned function_boundary (tree decl);
>
> and for decl == NULL return the "default" (the default hook implementation
> would just return MAX (FUNCTION_BOUNDARY, DECL_ALIGN)).  We'd call 
> function_boundary (NULL) from tree.c and later from varasm.c query
> it again, but this time with the decl specified.
>
> And yes, you'd lower your default function_boundary.

What I don't like about this is the function_boundary (NULL) bit.
If having function_boundary (NULL) lower than function_boundary (decl)
is dangerous, that implies that we're using the function_boundary (NULL)
somewhere, which in itself seems bad.

How about initialising the DECL_ALIGN to:

    (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
     ? 2 * BITS_PER_UNIT : BITS_PER_UNIT)

instead of function_boundary (NULL)?  That way we might even be able to
rely on DECL_ALIGN in get_object_alignment_2.

Richard