| Message ID | 1446205692-22412-6-git-send-email-tbsaunde+gcc@tbsaunde.org |
|---|---|
| State | New |
| Headers | show |
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. Bernd
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. 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). 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) Richard. > > Bernd
> > 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. If necessary, then yes that would be a better solution. Even just keeping the abstraction of the macro and putting definitions of it inside #ifdef at the top of the file would be an improvement over the submitted patch, but IMO still not really compatible with our standards. Bernd
On Fri, Oct 30, 2015 at 1:28 PM, Bernd Schmidt <bschmidt@redhat.com> wrote: >> >> 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. > > > If necessary, then yes that would be a better solution. > > Even just keeping the abstraction of the macro and putting definitions of it > inside #ifdef at the top of the file would be an improvement over the > submitted patch, but IMO still not really compatible with our standards. I agree, that would make the source of the copy more obvious. Still libobjc is beyond what I consider compatible with our standards (just look at the hoops it jumps through to make the target macros work!) Richard. > > Bernd >
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
On 10/30/2015 01:47 PM, Richard Biener wrote: > On Fri, Oct 30, 2015 at 1:28 PM, Bernd Schmidt <bschmidt@redhat.com> wrote: >>> >>> 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. >> >> >> If necessary, then yes that would be a better solution. >> >> Even just keeping the abstraction of the macro and putting definitions of it >> inside #ifdef at the top of the file would be an improvement over the >> submitted patch, but IMO still not really compatible with our standards. > > I agree, that would make the source of the copy more obvious. If we go down that path, there's another minimum requirement - adjust the docs to say that the macro must be defined in two places. That's my main worry, someone creating a new port, completely oblivious that they'd have to modify library code for it to work. Bernd
On Fri, Oct 30, 2015 at 9:11 PM, Bernd Schmidt <bschmidt@redhat.com> wrote: > On 10/30/2015 01:47 PM, Richard Biener wrote: >> >> On Fri, Oct 30, 2015 at 1:28 PM, Bernd Schmidt <bschmidt@redhat.com> >> wrote: >>>> >>>> >>>> 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. >>> >>> >>> >>> If necessary, then yes that would be a better solution. >>> >>> Even just keeping the abstraction of the macro and putting definitions of >>> it >>> inside #ifdef at the top of the file would be an improvement over the >>> submitted patch, but IMO still not really compatible with our standards. >> >> >> I agree, that would make the source of the copy more obvious. > > > If we go down that path, there's another minimum requirement - adjust the > docs to say that the macro must be defined in two places. That's my main > worry, someone creating a new port, completely oblivious that they'd have to > modify library code for it to work. That happens already with respect to libffi anyways. libffi knows the inter-working of how ABIs work and has many target specific assembly code. If you look at libsanitizer, the code is even worse with all the macros that need to be changed and I rather not go down that route where we have processor specific #ifdefs in the code. Yes libobjc is mostly just works but going the route of libsanitizer is just wrong and makes a really bad precedent of target libraries. Even libgomp has some target specific headers and is very short of the number of headers/defines you need to change to it working (most of the time none). Thanks, Andrew > > > Bernd
On Sat, Oct 31, 2015 at 03:02:07PM +0800, Andrew Pinski wrote: > On Fri, Oct 30, 2015 at 9:11 PM, Bernd Schmidt <bschmidt@redhat.com> wrote: > > On 10/30/2015 01:47 PM, Richard Biener wrote: > >> > >> On Fri, Oct 30, 2015 at 1:28 PM, Bernd Schmidt <bschmidt@redhat.com> > >> wrote: > >>>> > >>>> > >>>> 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. > >>> > >>> > >>> > >>> If necessary, then yes that would be a better solution. > >>> > >>> Even just keeping the abstraction of the macro and putting definitions of > >>> it > >>> inside #ifdef at the top of the file would be an improvement over the > >>> submitted patch, but IMO still not really compatible with our standards. > >> > >> > >> I agree, that would make the source of the copy more obvious. > > > > > > If we go down that path, there's another minimum requirement - adjust the > > docs to say that the macro must be defined in two places. That's my main > > worry, someone creating a new port, completely oblivious that they'd have to > > modify library code for it to work. > > That happens already with respect to libffi anyways. libffi knows the > inter-working of how ABIs work and has many target specific assembly > code. I think basically any library of sufficient size will end up having target specific code somewhere, and will require some amount of effort to port to new arches, and if it pokes at low level details it'll probably take even more. > If you look at libsanitizer, the code is even worse with all the > macros that need to be changed and I rather not go down that route > where we have processor specific #ifdefs in the code. I've never worked with libsanitizer, but #ifdef really isn't different from macros and a giant shell case on $target accept that has more indirection. libgcc uses a config/ and it is still some rather bad code, and don't get me started on the stuff people have stuffed into macros in gcc/config/. > Yes libobjc is mostly just works but going the route of libsanitizer > is just wrong and makes a really bad precedent of target libraries. That seems to be a conclusion without any real justification. It may well be true libsanitizer could be organized better, but just saying its "wrong" isn't very convincing ;) thanks Trev > Even libgomp has some target specific headers and is very short of the > number of headers/defines you need to change to it working (most of > the time none). > > Thanks, > Andrew > > > > > > > Bernd
diff --git a/libobjc/encoding.c b/libobjc/encoding.c index 7438d64..0fbfd39 100644 --- a/libobjc/encoding.c +++ b/libobjc/encoding.c @@ -1171,8 +1171,12 @@ objc_layout_structure_next_member (struct objc_struct_layout *layout) #elif defined __tilegx__ desired_align = MIN (desired_align, 128); #endif -#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 /* Record must have at least as much alignment as any field.
From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org> Not many targets define this macro in ways that do something in libojc, so it seems to make sense to just inline the few definitions that do do something. libobjc/ChangeLog: 2015-10-30 Trevor Saunders <tbsaunde+gcc@tbsaunde.org> PR libobjc/24775 * encoding.c (objc_layout_structure_next_member): Remove usage of ADJUST_FIELD_ALIGN. --- libobjc/encoding.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)