| Message ID | 9C2B0543-065F-4BFB-B3E1-C2E138A54927@sandoe-acoustics.co.uk |
|---|---|
| State | New |
| Headers | show |
On Jul 24, 2010, at 1:36 AM, IainS wrote: > We need to deal with the fact that the darwin ppc64 ABI is defined by an > earlier version of gcc, with the property that it always applied alignment > adjustments to the va-args (even for zero-sized types). > OK for trunk and 4.5 when it re-opens? I'm fine with this, David, any objections? Also, as memory serves there is one more fix from last year that fixes the abi so that it is self consistent from last year. I think it fixes a failing struct abi testcase as I recall.
On 24 Jul 2010, at 10:45, Mike Stump wrote: > On Jul 24, 2010, at 1:36 AM, IainS wrote: >> We need to deal with the fact that the darwin ppc64 ABI is defined >> by an >> earlier version of gcc, with the property that it always applied >> alignment >> adjustments to the va-args (even for zero-sized types). > >> OK for trunk and 4.5 when it re-opens? > > I'm fine with this, David, any objections? > > > Also, as memory serves there is one more fix from last year that > fixes the abi so that it is self consistent from last year. I think > it fixes a failing struct abi testcase as I recall. hm. All the test-cases from the PRs and gxx.dg/compat and gxx.dg/struct- layout-1 now pass ... (less breakage to struct-by-value-1.c caused last month, which I'm trying to track down). The "RS6000_STRUCT_HACK" no longer seems necessary (but I might be mistaken) .. can you identify the PR, or Radar? cheers, Iain
On Sat, Jul 24, 2010 at 09:36:14AM +0100, IainS wrote: > Index: gcc/config/rs6000/rs6000.c > =================================================================== > --- gcc/config/rs6000/rs6000.c (revision 162457) > +++ gcc/config/rs6000/rs6000.c (working copy) > @@ -8931,6 +8995,51 @@ rs6000_gimplify_va_arg (tree valist, tree type, > gi > return build_va_arg_indirect_ref (t); > } > > +#if TARGET_MACHO Could we avoid introducing more #if TARGET_MACHO where it's not necessary? It should be sufficient to say something like: if (TARGET_MACHO && rs6000_darwin_abi && integer_zerop (TYPE_SIZE (type))) ... and the compiler will DTRT and fold out that code for non-Darwin targets. (This comment applies equally to your other darwin64 patch.) -Nathan
On 24 Jul 2010, at 11:13, Nathan Froyd wrote: > On Sat, Jul 24, 2010 at 09:36:14AM +0100, IainS wrote: >> Index: gcc/config/rs6000/rs6000.c >> =================================================================== >> --- gcc/config/rs6000/rs6000.c (revision 162457) >> +++ gcc/config/rs6000/rs6000.c (working copy) >> @@ -8931,6 +8995,51 @@ rs6000_gimplify_va_arg (tree valist, tree >> type, >> gi >> return build_va_arg_indirect_ref (t); >> } >> >> +#if TARGET_MACHO > > Could we avoid introducing more #if TARGET_MACHO where it's not > necessary? It should be sufficient to say something like: > > if (TARGET_MACHO > && rs6000_darwin_abi > && integer_zerop (TYPE_SIZE (type))) > ... > > and the compiler will DTRT and fold out that code for non-Darwin > targets. (This comment applies equally to your other darwin64 patch.) sure, if that's the preference, no problem. I guess I was thinking that it would be better to eliminate the code at pre-processing cheers, Iain
Mike, David, On 24 Jul 2010, at 10:45, Mike Stump wrote: > Also, as memory serves there is one more fix from last year that > fixes the abi so that it is self consistent from last year. I think > it fixes a failing struct abi testcase as I recall. OK, I found it (the struct hack is needed after all, in fact). FWIW, The fail of gcc.target/powerpc/ppc64-abi-1.c is misleading - I think that is a situation where the test-case needs some tweaking. The darwin64-abi.c check from 4.2.1 (or LLVM) is the right thing to use... ... but I have no idea what license it is covered by and, therefore, whether we can import it... so for now I'm using it outside the gcc tree. ==== There will follow a part #3 (I can hack a fix... now I need to find the Right Way). Part 3 will apply on top of parts 1 & 2 .. so those patches stand. cheers Iain
On Sat, Jul 24, 2010 at 11:06 AM, IainS <developer@sandoe-acoustics.co.uk> wrote: > Mike, David, > > On 24 Jul 2010, at 10:45, Mike Stump wrote: >> >> Also, as memory serves there is one more fix from last year that fixes the >> abi so that it is self consistent from last year. I think it fixes a >> failing struct abi testcase as I recall. > > OK, I found it (the struct hack is needed after all, in fact). > > FWIW, The fail of gcc.target/powerpc/ppc64-abi-1.c is misleading - I think > that is a situation where the test-case needs some tweaking. > > The darwin64-abi.c check from 4.2.1 (or LLVM) is the right thing to use... > ... but I have no idea what license it is covered by and, therefore, > whether we can import it... so for now I'm using it outside the gcc tree. > > ==== > > There will follow a part #3 (I can hack a fix... now I need to find the > Right Way). > > Part 3 will apply on top of parts 1 & 2 .. so those patches stand. Darwin-specific patches approved by Mike are fine with me. Thanks, David
On Sat, Jul 24, 2010 at 12:43:52PM +0100, IainS wrote: > On 24 Jul 2010, at 11:13, Nathan Froyd wrote: >> Could we avoid introducing more #if TARGET_MACHO where it's not >> necessary? It should be sufficient to say something like: >> >> if (TARGET_MACHO >> && rs6000_darwin_abi >> && integer_zerop (TYPE_SIZE (type))) >> ... >> >> and the compiler will DTRT and fold out that code for non-Darwin >> targets. (This comment applies equally to your other darwin64 patch.) > > sure, if that's the preference, no problem. > I guess I was thinking that it would be better to eliminate the code at > pre-processing It does slightly speed things up (I doubt that you'd notice the speedup, really). The rationale for using: if (TARGET_MACHO) versus #if TARGET_MACHO is that with the former, the compiler will validate that the code syntax-checks and type-checks even when compiling for non-Darwin targets. This extra checking makes it slightly harder to inadvertently break things. -Nathan
On 26 Jul 2010, at 14:33, Nathan Froyd wrote: > On Sat, Jul 24, 2010 at 12:43:52PM +0100, IainS wrote: >> On 24 Jul 2010, at 11:13, Nathan Froyd wrote: >>> Could we avoid introducing more #if TARGET_MACHO where it's not >>> necessary? It should be sufficient to say something like: >>> >>> if (TARGET_MACHO >>> && rs6000_darwin_abi >>> && integer_zerop (TYPE_SIZE (type))) >>> ... >>> >>> and the compiler will DTRT and fold out that code for non-Darwin >>> targets. (This comment applies equally to your other darwin64 >>> patch.) >> >> sure, if that's the preference, no problem. >> I guess I was thinking that it would be better to eliminate the >> code at >> pre-processing > > It does slightly speed things up (I doubt that you'd notice the > speedup, > really). The rationale for using: > > if (TARGET_MACHO) > > versus > > #if TARGET_MACHO > > is that with the former, the compiler will validate that the code > syntax-checks and type-checks even when compiling for non-Darwin > targets. This extra checking makes it slightly harder to > inadvertently > break things. OK. I'll do a second pass through (after sorting out part #3 of the ABI fixes). Apropos extending this to its logical conclusion: I suspect that the only viable 'end-game' at present is to macro-ize the machopic* calls, exposing the whole of the machopic workings would probably carry too much weight. (that comment would apply equally to i386.c, which also uses that machopic common code). Iain
On 24 Jul 2010, at 16:10, David Edelsohn wrote: > On Sat, Jul 24, 2010 at 11:06 AM, IainS > <developer@sandoe-acoustics.co.uk> wrote: >> Mike, David, >> >> On 24 Jul 2010, at 10:45, Mike Stump wrote: >>> >>> Also, as memory serves there is one more fix from last year that >>> fixes the >>> abi so that it is self consistent from last year. I think it >>> fixes a >>> failing struct abi testcase as I recall. >> >> OK, I found it (the struct hack is needed after all, in fact). >> >> FWIW, The fail of gcc.target/powerpc/ppc64-abi-1.c is misleading - >> I think >> that is a situation where the test-case needs some tweaking. >> >> The darwin64-abi.c check from 4.2.1 (or LLVM) is the right thing to >> use... >> ... but I have no idea what license it is covered by and, >> therefore, >> whether we can import it... so for now I'm using it outside the gcc >> tree. >> >> ==== >> >> There will follow a part #3 (I can hack a fix... now I need to find >> the >> Right Way). >> >> Part 3 will apply on top of parts 1 & 2 .. so those patches stand. > > Darwin-specific patches approved by Mike are fine with me part 1 is r162567 part 2 (as amended with Nathan's suggestion, copied below) is r162568 (changelog for #2 is 162569) cheers, Iain amended part 2/2 /* We need to deal with the fact that the darwin ppc64 ABI is defined by an earlier version of gcc, with the property that it always applied alignment adjustments to the va-args (even for zero-sized types). The cheapest way to deal with this is to replicate the effect of the part of std_gimplify_va_arg_expr that carries out the align adjust, for the case of relevance. We don't need to check for pass-by-reference because of the test above. We can return a simplifed answer, since we know there's no offset to add. */ if (TARGET_MACHO && rs6000_darwin64_abi && integer_zerop (TYPE_SIZE (type))) { unsigned HOST_WIDE_INT align, boundary; tree valist_tmp = get_initialized_tmp_var (valist, pre_p, NULL); align = PARM_BOUNDARY / BITS_PER_UNIT; boundary = FUNCTION_ARG_BOUNDARY (TYPE_MODE (type), type); if (boundary > MAX_SUPPORTED_STACK_ALIGNMENT) boundary = MAX_SUPPORTED_STACK_ALIGNMENT; boundary /= BITS_PER_UNIT; if (boundary > align) { tree t ; /* This updates arg ptr by the amount that would be necessary to align the zero-sized (but not zero-alignment) item. */ t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp, fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (valist), valist_tmp, size_int (boundary - 1))); gimplify_and_add (t, pre_p); t = fold_convert (sizetype, valist_tmp); t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp, fold_convert (TREE_TYPE (valist), fold_build2 (BIT_AND_EXPR, sizetype, t, size_int (-boundary)))); t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist, t); gimplify_and_add (t, pre_p); } /* Since it is zero-sized there's no increment for the item itself. */ valist_tmp = fold_convert (build_pointer_type (type), valist_tmp); return build_va_arg_indirect_ref (valist_tmp); }
Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 162457) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -8931,6 +8995,51 @@ rs6000_gimplify_va_arg (tree valist, tree type, gi return build_va_arg_indirect_ref (t); } +#if TARGET_MACHO + /* We need to deal with the fact that the darwin ppc64 ABI is defined by an + earlier version of gcc, with the property that it always applied alignment + adjustments to the va-args (even for zero-sized types). The cheapest way + to deal with this is to replicate the effect of the part of + std_gimplify_va_arg_expr that carries out the align adjust, for the case + of relevance. + We don't need to check for pass-by-reference because of the test above. + We can return a simplifed answer, since we know there's no offset to add. */ + + if (rs6000_darwin64_abi + && integer_zerop (TYPE_SIZE (type))) + { + unsigned HOST_WIDE_INT align, boundary; + tree valist_tmp = get_initialized_tmp_var (valist, pre_p, NULL); + align = PARM_BOUNDARY / BITS_PER_UNIT; + boundary = FUNCTION_ARG_BOUNDARY (TYPE_MODE (type), type); + if (boundary > MAX_SUPPORTED_STACK_ALIGNMENT) + boundary = MAX_SUPPORTED_STACK_ALIGNMENT; + boundary /= BITS_PER_UNIT; + if (boundary > align) + { + tree t ; + /* This updates arg ptr by the amount that would be necessary + to align the zero-sized (but not zero-alignment) item. */ + t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp, + fold_build2 (POINTER_PLUS_EXPR, + TREE_TYPE (valist), + valist_tmp, size_int (boundary - 1))); + gimplify_and_add (t, pre_p); + + t = fold_convert (sizetype, valist_tmp); + t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp, + fold_convert (TREE_TYPE (valist), + fold_build2 (BIT_AND_EXPR, sizetype, t, + size_int (-boundary)))); + t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist, t); + gimplify_and_add (t, pre_p); + } + /* Since it is zero-sized there's no increment for the item itself. */
Hi, We need to deal with the fact that the darwin ppc64 ABI is defined by an earlier version of gcc, with the property that it always applied alignment adjustments to the va-args (even for zero-sized types). The cheapest way to deal with this is to replicate the effect of the part of std_gimplify_va_arg_expr () that carries out the align adjust, for the case of relevance. I've made it such that there is no impact at all for non-darwin. The impact for darwin is restricted to the single m64 ABI case. OK for trunk and 4.5 when it re-opens? Iain gcc/ PR target/29090 * config/rs6000/rs6000.c (rs6000_gimplify_va_arg): Special-case the Darwin64 ABI, for zero-sized objects. + valist_tmp = fold_convert (build_pointer_type (type), valist_tmp); + return build_va_arg_indirect_ref (valist_tmp); + } +#endif + if (DEFAULT_ABI != ABI_V4) { if (targetm.calls.split_complex_arg && TREE_CODE (type) == COMPLEX_TYPE)