| Message ID | 537A5DA9.9010905@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On 05/19/2014 01:38 PM, Sandra Loosemore wrote: > > 2014-05-19 Iain Sandoe <iain@codesourcery.com> > Catherine Moore <clm@codesourcery.com> > Sandra Loosemore <sandra@codesourcery.com> > > gcc/ > * config/mips/mips.c (mips_set_current_function): Choose > function alignment once the current mode is known. > > gcc/testsuite/ > * gcc.target/mips/umips-align-1.c: New. > * gcc.target/mips/umips-align-2.c: New. > * gcc.target/mips/umips-align-3.c: New. > * gcc.target/mips/mips.exp: Add interlink-compressed to > -mfoo/-mno-foo options. Ping? https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01536.html -Sandra
Sandra Loosemore <sandra@codesourcery.com> writes: > On 05/19/2014 01:38 PM, Sandra Loosemore wrote: >> >> 2014-05-19 Iain Sandoe <iain@codesourcery.com> >> Catherine Moore <clm@codesourcery.com> >> Sandra Loosemore <sandra@codesourcery.com> >> >> gcc/ >> * config/mips/mips.c (mips_set_current_function): Choose >> function alignment once the current mode is known. >> >> gcc/testsuite/ >> * gcc.target/mips/umips-align-1.c: New. >> * gcc.target/mips/umips-align-2.c: New. >> * gcc.target/mips/umips-align-3.c: New. >> * gcc.target/mips/mips.exp: Add interlink-compressed to >> -mfoo/-mno-foo options. > > Ping? > > https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01536.html > > -Sandra FAOD, I wasn't commenting because I still think it's the wrong place but still don't have a specific counter-suggestion. mips_set_current_function is potentially called many times for the same function but setting the alignment seems like something that should only happen once. I think it could potentially mean that alignment tests against the function address could be optimised away based on the FUNCTION_BOUNDARY before mips_set_current_function is called. As a strawman, maybe adding a new target hook to cgraph_create_node would work? Hopefully that'll prompt someone to say how stupid that idea is and say what the right way of doing it would be. Thanks, Richard
On 05/28/2014 01:09 PM, Richard Sandiford wrote: > Sandra Loosemore <sandra@codesourcery.com> writes: > >> On 05/19/2014 01:38 PM, Sandra Loosemore wrote: >>> >>> 2014-05-19 Iain Sandoe <iain@codesourcery.com> >>> Catherine Moore <clm@codesourcery.com> >>> Sandra Loosemore <sandra@codesourcery.com> >>> >>> gcc/ >>> * config/mips/mips.c (mips_set_current_function): Choose >>> function alignment once the current mode is known. >>> >>> gcc/testsuite/ >>> * gcc.target/mips/umips-align-1.c: New. >>> * gcc.target/mips/umips-align-2.c: New. >>> * gcc.target/mips/umips-align-3.c: New. >>> * gcc.target/mips/mips.exp: Add interlink-compressed to >>> -mfoo/-mno-foo options. >> >> Ping? >> >> https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01536.html >> >> -Sandra > > FAOD, I wasn't commenting because I still think it's the wrong place but > still don't have a specific counter-suggestion. mips_set_current_function > is potentially called many times for the same function but setting the > alignment seems like something that should only happen once. I think it > could potentially mean that alignment tests against the function address > could be optimised away based on the FUNCTION_BOUNDARY before > mips_set_current_function is called. > > As a strawman, maybe adding a new target hook to cgraph_create_node > would work? Hmmmm, why there? > Hopefully that'll prompt someone to say how stupid that > idea is and say what the right way of doing it would be. If the implementation in the current patch is considered too stupid, I'd rather spend my time implementing a plausibly correct version on my next try instead of another stupid one. My best guess is that this belongs somewhere in stor-layout.c, but the comments on layout_decl explicitly say FUNCTION_DECLs are not handled there, and I am not so familiar with the code as to be able to identify all the places where FUNCTION_DECLs ought to get their storage laid out but are not currently having it done. So that's probably a stupid idea, too.... :-( -Sandra
Sandra Loosemore <sandra@codesourcery.com> writes: > On 05/28/2014 01:09 PM, Richard Sandiford wrote: >> Sandra Loosemore <sandra@codesourcery.com> writes: >> >>> On 05/19/2014 01:38 PM, Sandra Loosemore wrote: >>>> >>>> 2014-05-19 Iain Sandoe <iain@codesourcery.com> >>>> Catherine Moore <clm@codesourcery.com> >>>> Sandra Loosemore <sandra@codesourcery.com> >>>> >>>> gcc/ >>>> * config/mips/mips.c (mips_set_current_function): Choose >>>> function alignment once the current mode is known. >>>> >>>> gcc/testsuite/ >>>> * gcc.target/mips/umips-align-1.c: New. >>>> * gcc.target/mips/umips-align-2.c: New. >>>> * gcc.target/mips/umips-align-3.c: New. >>>> * gcc.target/mips/mips.exp: Add interlink-compressed to >>>> -mfoo/-mno-foo options. >>> >>> Ping? >>> >>> https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01536.html >>> >>> -Sandra >> >> FAOD, I wasn't commenting because I still think it's the wrong place but >> still don't have a specific counter-suggestion. mips_set_current_function >> is potentially called many times for the same function but setting the >> alignment seems like something that should only happen once. I think it >> could potentially mean that alignment tests against the function address >> could be optimised away based on the FUNCTION_BOUNDARY before >> mips_set_current_function is called. >> >> As a strawman, maybe adding a new target hook to cgraph_create_node >> would work? > > Hmmmm, why there? I just thought trying to trap it at cgraph hand-off time would be late enough to know the ISA mode of the function but early enough to catch inter-function references. >> Hopefully that'll prompt someone to say how stupid that >> idea is and say what the right way of doing it would be. > > If the implementation in the current patch is considered too stupid, I'd > rather spend my time implementing a plausibly correct version on my next > try instead of another stupid one. > > My best guess is that this belongs somewhere in stor-layout.c, but the > comments on layout_decl explicitly say FUNCTION_DECLs are not handled > there, and I am not so familiar with the code as to be able to identify > all the places where FUNCTION_DECLs ought to get their storage laid out > but are not currently having it done. So that's probably a stupid idea, > too.... :-( I'll try asking on IRC next week. Thanks, Richard
Sandra Loosemore <sandra@codesourcery.com> writes: > Catherine included an earlier version of this patch with the microMIPS > submission a couple years ago: > > https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00972.html > > Richard's response was: > >> Looks like the wrong place to do this. Please treat this as a separate >> patch and get a tree expert to comment. > > So, here is the separate patch, finally. :-) Can a tree expert > comment? I dug around and didn't see another obvious hook to set > function alignment in a target-specific way depending on attributes of > the function. > > Besides the new test cases, I regression-tested this on mips-sde-elf > using Mentor's usual assortment of multilibs, specifically including one > for microMIPS. OK, I asked Richi on IRC today. To recap, one of the things I was worried about was a test against the address, like (foo & 2) == 0, being optimised away before we set the alignment. Richi pointed out that my idea downthread about cgraph_create_node would also be too late to avoid that. Also, looking at it now, I see that we don't trust DECL_ALIGN on functions anyway. From get_object_alignment_2: /* Extract alignment information from the innermost object and possibly adjust bitpos and offset. */ if (TREE_CODE (exp) == FUNCTION_DECL) { /* Function addresses can encode extra information besides their alignment. However, if TARGET_PTRMEMFUNC_VBIT_LOCATION allows the low bit to be used as a virtual bit, we know that the address itself must be at least 2-byte aligned. */ if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn) align = 2 * BITS_PER_UNIT; } And since we use the low bit to encode the ISA mode on MIPS, the upshot is that we never assume any alignment for functions. So there's nothing to worry about after all. Richi suggested just changing the alignment at output time. I assume that would be a case of replacing the DECL_ALIGN in: /* Tell assembler to move to target machine's alignment for functions. */ align = floor_log2 (DECL_ALIGN (decl) / BITS_PER_UNIT); if (align > 0) { ASM_OUTPUT_ALIGN (asm_out_file, align); } with a hook. (Is that right?) Thanks, Richard
On Tue, 3 Jun 2014, Richard Sandiford wrote: > Sandra Loosemore <sandra@codesourcery.com> writes: > > Catherine included an earlier version of this patch with the microMIPS > > submission a couple years ago: > > > > https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00972.html > > > > Richard's response was: > > > >> Looks like the wrong place to do this. Please treat this as a separate > >> patch and get a tree expert to comment. > > > > So, here is the separate patch, finally. :-) Can a tree expert > > comment? I dug around and didn't see another obvious hook to set > > function alignment in a target-specific way depending on attributes of > > the function. > > > > Besides the new test cases, I regression-tested this on mips-sde-elf > > using Mentor's usual assortment of multilibs, specifically including one > > for microMIPS. > > OK, I asked Richi on IRC today. To recap, one of the things I was > worried about was a test against the address, like (foo & 2) == 0, > being optimised away before we set the alignment. Richi pointed > out that my idea downthread about cgraph_create_node would also be > too late to avoid that. Also, looking at it now, I see that we don't > trust DECL_ALIGN on functions anyway. From get_object_alignment_2: > > /* Extract alignment information from the innermost object and > possibly adjust bitpos and offset. */ > if (TREE_CODE (exp) == FUNCTION_DECL) > { > /* Function addresses can encode extra information besides their > alignment. However, if TARGET_PTRMEMFUNC_VBIT_LOCATION > allows the low bit to be used as a virtual bit, we know > that the address itself must be at least 2-byte aligned. */ > if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn) > align = 2 * BITS_PER_UNIT; > } > > And since we use the low bit to encode the ISA mode on MIPS, the upshot > is that we never assume any alignment for functions. So there's nothing > to worry about after all. > > Richi suggested just changing the alignment at output time. I assume > that would be a case of replacing the DECL_ALIGN in: > > /* Tell assembler to move to target machine's alignment for functions. */ > align = floor_log2 (DECL_ALIGN (decl) / BITS_PER_UNIT); > if (align > 0) > { > ASM_OUTPUT_ALIGN (asm_out_file, align); > } > > with a hook. (Is that right?) Yeah, kind of. Of course if DECL_ALIGN on function-decls is "unused" then we may as well initialize it to 1 in tree.c and at an appropriate stage adjust it to the result of a target hook invocation. Appropriate stage would be the above place (which means DECL_ALIGN is essentially "unused" for FUNCTION_DECLs). So ... can you massage the DECL_ALIGN macro to ICE on FUNCTION_DECLs and see where we access it? (generic code will possibly trip on it, but the question is is there any user that cares?) Richard.
On 06/04/2014 06:20 AM, Richard Biener wrote: > On Tue, 3 Jun 2014, Richard Sandiford wrote: > >> Richi suggested just changing the alignment at output time. I assume >> that would be a case of replacing the DECL_ALIGN in: >> >> /* Tell assembler to move to target machine's alignment for functions. */ >> align = floor_log2 (DECL_ALIGN (decl) / BITS_PER_UNIT); >> if (align > 0) >> { >> ASM_OUTPUT_ALIGN (asm_out_file, align); >> } >> >> with a hook. (Is that right?) > > Yeah, kind of. Of course if DECL_ALIGN on function-decls is "unused" > then we may as well initialize it to 1 in tree.c and at an appropriate > stage adjust it to the result of a target hook invocation. > > Appropriate stage would be the above place (which means DECL_ALIGN > is essentially "unused" for FUNCTION_DECLs). > > So ... can you massage the DECL_ALIGN macro to ICE on FUNCTION_DECLs > and see where we access it? (generic code will possibly trip on it, > but the question is is there any user that cares?) Well, offhand, I know that one of the places is in handle_aligned_attribute, in c-common/c-common.c, where we handle user-specified alignment attributes. Here we are potentially both reading and writing the DECL_ALIGN field of a FUNCTION_DECL. -Sandra
On Wed, 4 Jun 2014, Sandra Loosemore wrote: > On 06/04/2014 06:20 AM, Richard Biener wrote: > > On Tue, 3 Jun 2014, Richard Sandiford wrote: > > > > > Richi suggested just changing the alignment at output time. I assume > > > that would be a case of replacing the DECL_ALIGN in: > > > > > > /* Tell assembler to move to target machine's alignment for functions. > > > */ > > > align = floor_log2 (DECL_ALIGN (decl) / BITS_PER_UNIT); > > > if (align > 0) > > > { > > > ASM_OUTPUT_ALIGN (asm_out_file, align); > > > } > > > > > > with a hook. (Is that right?) > > > > Yeah, kind of. Of course if DECL_ALIGN on function-decls is "unused" > > then we may as well initialize it to 1 in tree.c and at an appropriate > > stage adjust it to the result of a target hook invocation. > > > > Appropriate stage would be the above place (which means DECL_ALIGN > > is essentially "unused" for FUNCTION_DECLs). > > > > So ... can you massage the DECL_ALIGN macro to ICE on FUNCTION_DECLs > > and see where we access it? (generic code will possibly trip on it, > > but the question is is there any user that cares?) > > Well, offhand, I know that one of the places is in handle_aligned_attribute, > in c-common/c-common.c, where we handle user-specified alignment attributes. > Here we are potentially both reading and writing the DECL_ALIGN field of a > FUNCTION_DECL. 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. Richard.
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? -Sandra
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...) Richard
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...) I'm willing to write/test a patch once you guys agree on the semantics of the new hook and are happy with how it would be used to solve this particular problem.... -Sandra
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. Richard. > I'm willing to write/test a patch once you guys agree on the semantics of the > new hook and are happy with how it would be used to solve this particular > problem....
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
On Fri, 6 Jun 2014, Richard Sandiford wrote: > 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. The other way around - function_boundary (NULL) should not be larger than function_boundary (decl). > 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. Not sure about that, but initializing that way certainly looks reasonable. Btw, maybe we should call function_boundary to properly initialize DECL_ALIGN earlier than output time, for example in cgraph_finalize_function. There might be an IPA pass sorting functions after their alignment to squeeze some more bits by reducing possible padding. Richard.
On 06/06/2014 01:44 AM, Richard Biener wrote: > On Fri, 6 Jun 2014, Richard Sandiford wrote: > > [snip] > >> 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. > > Not sure about that, but initializing that way certainly looks > reasonable. > > Btw, maybe we should call function_boundary to properly initialize > DECL_ALIGN earlier than output time, for example in > cgraph_finalize_function. There might be an IPA pass sorting > functions after their alignment to squeeze some more bits > by reducing possible padding. Hrmmmm. I was looking again at handle_aligned_attribute in c-common.c, which gives an error if you try to decrease the alignment from the target-specific default given by FUNCTION_BOUNDARY. If we don't invoke the new hook to set the default until after alignment attributes are handled, we'll have to duplicate that logic. More problematically, earlier in that file there is also code to handle the __alignof__ operator. When the operand is a function type, it uses FUNCTION_BOUNDARY; when the operand is a FUNCTION_DECL, it uses DECL_ALIGN. So, if we defer setting the target-specific default after the front end, we could potentially break all code that uses __alignof__ on functions.... :-( Like I mentioned before, my guess was that the "right" place to set DECL_ALIGN for functions is in layout_decl or some new equivalent for functions. Postponing it to output time, or some random optimization pass, seems likely to cause inconsistent behavior. -Sandra
Index: gcc/config/mips/mips.c =================================================================== --- gcc/config/mips/mips.c (revision 210478) +++ gcc/config/mips/mips.c (working copy) @@ -16822,6 +16822,13 @@ static void mips_set_current_function (tree fndecl) { mips_set_compression_mode (mips_get_compress_mode (fndecl)); + + if (fndecl + && TARGET_MICROMIPS + && optimize_size + && !TARGET_INTERLINK_COMPRESSED + && !DECL_USER_ALIGN (fndecl)) + DECL_ALIGN (fndecl) = 16; } /* Allocate a chunk of memory for per-function machine-dependent data. */ Index: gcc/testsuite/gcc.target/mips/umips-align-1.c =================================================================== --- gcc/testsuite/gcc.target/mips/umips-align-1.c (revision 0) +++ gcc/testsuite/gcc.target/mips/umips-align-1.c (revision 0) @@ -0,0 +1,14 @@ +/* { dg-options "(-mmicromips) -mno-interlink-compressed" } */ +/* { dg-skip-if "code size optimization" { *-*-* } { "*" } { "-Os" } } */ + +/* Check that microMIPS functions are aligned on 16-bit (2^1 byte) + boundaries with -Os. */ + +int MICROMIPS +f (int x) +{ + return x + 42; +} + +/* { dg-final { scan-assembler "\t\\.align\t1" } } */ + Index: gcc/testsuite/gcc.target/mips/umips-align-2.c =================================================================== --- gcc/testsuite/gcc.target/mips/umips-align-2.c (revision 0) +++ gcc/testsuite/gcc.target/mips/umips-align-2.c (revision 0) @@ -0,0 +1,13 @@ +/* { dg-options "(-mmicromips) -minterlink-compressed" } */ +/* { dg-skip-if "code size optimization" { *-*-* } { "*" } { "-Os" } } */ + +/* Check that -minterlink-compressed suppresses 16-bit alignment of + microMIPS functions with -Os. */ + +int MICROMIPS +f (int x) +{ + return x + 42; +} + +/* { dg-final { scan-assembler-not "\t\\.align\t1" } } */ Index: gcc/testsuite/gcc.target/mips/umips-align-3.c =================================================================== --- gcc/testsuite/gcc.target/mips/umips-align-3.c (revision 0) +++ gcc/testsuite/gcc.target/mips/umips-align-3.c (revision 0) @@ -0,0 +1,14 @@ +/* { dg-options "(-mmicromips) -mno-interlink-compressed" } */ +/* { dg-skip-if "code size optimization" { *-*-* } { "*" } { "-Os" } } */ + +/* Check that explicit alignment attribute suppresses 16-bit alignment of + microMIPS functions with -Os. */ + +int MICROMIPS +__attribute__ ((aligned (16))) +f (int x) +{ + return x + 42; +} + +/* { dg-final { scan-assembler-not "\t\\.align\t1" } } */ Index: gcc/testsuite/gcc.target/mips/mips.exp =================================================================== --- gcc/testsuite/gcc.target/mips/mips.exp (revision 210478) +++ gcc/testsuite/gcc.target/mips/mips.exp (working copy) @@ -260,6 +260,7 @@ foreach option { fix-r10000 fix-vr4130 gpopt + interlink-compressed local-sdata long-calls paired-single