Re: [PATCH 5/5] remove usage of ADJUST_FIELD_ALIGN in encoding.c

[Trevor Saunders <tbsaunde@tbsaunde.org>]

On Fri, Oct 30, 2015 at 01:16:16PM +0100, Richard Biener wrote:
> On Fri, Oct 30, 2015 at 1:06 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> > On 10/30/2015 12:48 PM, tbsaunde+gcc@tbsaunde.org wrote:
> >>
> >> -#ifdef ADJUST_FIELD_ALIGN
> >> -  desired_align = ADJUST_FIELD_ALIGN (type, desired_align);
> >> +#if defined __arc__ || defined _AIX
> >> +  if (TYPE_MODE (strip_array_types (TREE_TYPE (type))) == DFmode)
> >> +    desired_align = MIN (desired_align, 32);
> >> +#elif __POWERPC__ && __APPLE__
> >> +  if (desired_align != 128)
> >> +    desired_align = MIN (desired_align, 32);
> >>   #endif
> >
> >
> > No way. We never use this kind of test in target-independent code.
> 
> it's not target independent code.  Are you suggesting to add a config/
> to libobjc?  IMHO for a not really mantained frontend / target lib that's
> an excessive requirement.

Given the amount of target dependant stuff involved adding a config/
actually seems worse to me.  You are accomplishing the exact same thing,
but you need a whole lot more machinary to do it, and its hard to
understand what happens for any given platform.  Sure, if there was a
whole lot more target code doing something else might make sense, but
there isn't and I'm certainly not planning on adding more.  SO I think
its best to leave it this way and if someone wants to do substantial
work on libobjc in the future they can worry about that then.

btw the claim its never done just doesn't stand up either, look at the
__SPARC__ check this series removes, or the __MINGW__ check in gthr.h, or
even all the crap at the top of encoding.c that makes using these target
macros possible (it wouldn't actually suprise me if cleaning that up
ment doing this was a net reduction in target dependent code in
encoding.c).

If you want to be kind of sad I discovered
https://github.com/gnustep/libobjc2 while looking at this, so it seems
like many of the possible users of libobjc may even be not using it.

> For any such replacements as in the patch I suggest to at least keep a comment
> before it indicating the origin of the inlined vairants (in this case refer to
> ADJUST_FIELD_ALIGN).

That seems fairly reasonable, I'd kind of worry about them getting out
of date, but i guess it at least gives a place to start looking for an
explanation.

> In general I'm happy with this kind of patches (maybe not the
> BIGGEST_FIELD_ALIGN
> one which could be made a CPP macro when -fbuilding-libgcc)

I considered that, but the only targets that define
BIGGEST_FIELD_ALIGNMENT  for purposes other than IN_TARGET_LIBS hacks
were v850, vax, tilegx, and tilepro so considering
BIGGEST_FIELD_ALIGNMENT kind of dupplicates ADJUST_FIELD_ALIGN my
conclusion was it would make more sense to not do
that.  I'm thinking it makes sense to instead just merge
BIGGEST_FIELD_ALIGNMENT into ADJUST_FIELD_ALIGN, but adding a predefined
macro would make that harder.

Trev

> 
> Richard.
> 
> >
> > Bernd