* [RFC] Disabling ICF for interrupt functions @ 2019-07-19 12:45 Jozef Lawrynowicz 2019-07-19 13:32 ` Alexander Monakov 2019-07-26 17:39 ` Richard Sandiford 0 siblings, 2 replies; 8+ messages in thread From: Jozef Lawrynowicz @ 2019-07-19 12:45 UTC (permalink / raw) To: gcc For MSP430, the folding of identical functions marked with the "interrupt" attribute by -fipa-icf-functions results in wrong code being generated. Interrupts have different calling conventions than regular functions, so inserting a CALL from one identical interrupt to another is not correct and will result in stack corruption. I imagine there are other targets that also have different calling conventions for interrupt functions compared to regular functions, and so folding them would be undesirable. Therefore, I would appreciate some feedback as to whether it would be welcomed to fix this in a generic way or if I should just keep it MSP430 specific. 1. MSP430 specific Add the "no_icf" attribute to functions marked with the "interrupt" attribute when processing them in the backend. 2. Target Hook Add a DEFHOOKPOD (e.g. TARGET_NO_INTERRUPT_ICF) which controls whether ICF is disabled for functions with the interrupt attribute (in gcc/ipa-icf.c, where "no_icf" attribute is processed). 3. Always on Same as 2. but without the hook implementation - just check for the interrupt attribute and disable ICF if it is present. I'm personally leaning towards option 2, target hook, since other targets may benefit from this. Thanks, Jozef ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Disabling ICF for interrupt functions 2019-07-19 12:45 [RFC] Disabling ICF for interrupt functions Jozef Lawrynowicz @ 2019-07-19 13:32 ` Alexander Monakov 2019-07-22 17:01 ` Jozef Lawrynowicz 2019-07-26 17:39 ` Richard Sandiford 1 sibling, 1 reply; 8+ messages in thread From: Alexander Monakov @ 2019-07-19 13:32 UTC (permalink / raw) To: Jozef Lawrynowicz; +Cc: gcc On Fri, 19 Jul 2019, Jozef Lawrynowicz wrote: > For MSP430, the folding of identical functions marked with the "interrupt" > attribute by -fipa-icf-functions results in wrong code being generated. > Interrupts have different calling conventions than regular functions, so > inserting a CALL from one identical interrupt to another is not correct and > will result in stack corruption. But ICF by creating an alias would be fine, correct? As I understand, the real issue here is that gcc does not know how to correctly emit a call to "interrupt" functions (because they have unusual ABI and exist basically to have their address stored somewhere). So I think the solution shouldn't be in disabling ICF altogether, but rather in adding a way to recognize that a function has quasi-unknown ABI and thus not directly callable (so any other optimization can see that it may not emit a call to this function), then teaching ICF to check that when deciding to fold by creating a wrapper. (would it be possible to tell ICF that addresses of interrupt functions are not significant so it can fold them by creating aliases?) Alexander ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Disabling ICF for interrupt functions 2019-07-19 13:32 ` Alexander Monakov @ 2019-07-22 17:01 ` Jozef Lawrynowicz 2019-07-22 18:50 ` Alexander Monakov 0 siblings, 1 reply; 8+ messages in thread From: Jozef Lawrynowicz @ 2019-07-22 17:01 UTC (permalink / raw) To: Alexander Monakov; +Cc: gcc Hi, On Fri, 19 Jul 2019 16:32:21 +0300 (MSK) Alexander Monakov <amonakov@ispras.ru> wrote: > On Fri, 19 Jul 2019, Jozef Lawrynowicz wrote: > > > For MSP430, the folding of identical functions marked with the "interrupt" > > attribute by -fipa-icf-functions results in wrong code being generated. > > Interrupts have different calling conventions than regular functions, so > > inserting a CALL from one identical interrupt to another is not correct and > > will result in stack corruption. > > But ICF by creating an alias would be fine, correct? As I understand, the > real issue here is that gcc does not know how to correctly emit a call to > "interrupt" functions (because they have unusual ABI and exist basically to > have their address stored somewhere). Yes I presume in most cases an alias would be ok. It's just that users sometimes do funky things with interrupt functions to achieve the best possible performance for their programs, so I wouldn't want to rule out that identical interrupts may need distinct addresses in some situations. I cannot think of a use case for that right now though. So having the option to disable it somehow would be desirable. > > So I think the solution shouldn't be in disabling ICF altogether, but rather > in adding a way to recognize that a function has quasi-unknown ABI and thus > not directly callable (so any other optimization can see that it may not emit > a call to this function), then teaching ICF to check that when deciding to > fold by creating a wrapper. I agree, this is a nice suggestion. "call" instructions should be not be allowed to be generated at all for MSP430 (and whichever other targets) interrupt functions. Whether that be coming from the user explicitly calling the interrupt from their code, or GCC generating the call. This would have to be caught at the point that an optimization pass first considers inserting a CALL to the interrupt, i.e., if the machine description tries to prevent the generation of a call to an interrupt function once the RTL has been generated (e.g. by blanking on the define_expand for "call"), we are going to have ICEs/wrong code generated a lot of the time. Particularly in the case originally mentioned here - there would be an empty interrupt function. > > (would it be possible to tell ICF that addresses of interrupt functions are > not significant so it can fold them by creating aliases?) I'll take a look. Thanks, Jozef > > Alexander ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Disabling ICF for interrupt functions 2019-07-22 17:01 ` Jozef Lawrynowicz @ 2019-07-22 18:50 ` Alexander Monakov 0 siblings, 0 replies; 8+ messages in thread From: Alexander Monakov @ 2019-07-22 18:50 UTC (permalink / raw) To: Jozef Lawrynowicz; +Cc: gcc On Mon, 22 Jul 2019, Jozef Lawrynowicz wrote: > This would have to be caught at the point that an optimization pass > first considers inserting a CALL to the interrupt, i.e., if the machine > description tries to prevent the generation of a call to an interrupt function > once the RTL has been generated (e.g. by blanking on the define_expand for > "call"), we are going to have ICEs/wrong code generated a lot of the time. > Particularly in the case originally mentioned here - there would be an empty > interrupt function. Yeah, I imagine it would need to be a new target hook direct_call_allowed_p receiving a function decl, or something like that. > > (would it be possible to tell ICF that addresses of interrupt functions are > > not significant so it can fold them by creating aliases?) > > I'll take a look. Sorry, I didn't say explicitly, but that was meant more as a remark to IPA maintainers: currently in GCC "address taken" implies "address significant", so "address not significant" would have to be a new attribute, or a new decl bit (maybe preferable for languages where function addresses are not significant by default). Alexander ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Disabling ICF for interrupt functions 2019-07-19 12:45 [RFC] Disabling ICF for interrupt functions Jozef Lawrynowicz 2019-07-19 13:32 ` Alexander Monakov @ 2019-07-26 17:39 ` Richard Sandiford 2019-07-30 13:41 ` Jozef Lawrynowicz 1 sibling, 1 reply; 8+ messages in thread From: Richard Sandiford @ 2019-07-26 17:39 UTC (permalink / raw) To: Jozef Lawrynowicz; +Cc: gcc [Catching up after being away, sorry if this has already been resolved.] Jozef Lawrynowicz <jozef.l@mittosystems.com> writes: > For MSP430, the folding of identical functions marked with the "interrupt" > attribute by -fipa-icf-functions results in wrong code being generated. > Interrupts have different calling conventions than regular functions, so > inserting a CALL from one identical interrupt to another is not correct and > will result in stack corruption. > > I imagine there are other targets that also have different calling conventions > for interrupt functions compared to regular functions, and so folding them > would be undesirable. > > Therefore, I would appreciate some feedback as to whether it would be welcomed > to fix this in a generic way or if I should just keep it MSP430 specific. > > 1. MSP430 specific > Add the "no_icf" attribute to functions marked with the "interrupt" attribute > when processing them in the backend. > > 2. Target Hook > Add a DEFHOOKPOD (e.g. TARGET_NO_INTERRUPT_ICF) which controls whether ICF is > disabled for functions with the interrupt attribute (in gcc/ipa-icf.c, where > "no_icf" attribute is processed). This sounds like it should be a question about two functions rather than one, i.e.: can functions A and B be merged together if they appear to be identical at some level? I think for most targets, merging two identical interrupt functions would be OK. It's merging an interrupt and non-interrupt function that's the problem. So maybe we need something like targetm.can_inline_p, but for merging rather than inlining? The function would need to be transitive of course. Thanks, Richard > 3. Always on > Same as 2. but without the hook implementation - just check for the interrupt > attribute and disable ICF if it is present. > > I'm personally leaning towards option 2, target hook, since other targets may > benefit from this. > > Thanks, > Jozef ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Disabling ICF for interrupt functions 2019-07-26 17:39 ` Richard Sandiford @ 2019-07-30 13:41 ` Jozef Lawrynowicz 2019-07-31 9:50 ` Richard Biener 0 siblings, 1 reply; 8+ messages in thread From: Jozef Lawrynowicz @ 2019-07-30 13:41 UTC (permalink / raw) To: Richard Sandiford; +Cc: gcc, amonakov On Fri, 26 Jul 2019 18:39:50 +0100 Richard Sandiford <richard.sandiford@arm.com> wrote: > [Catching up after being away, sorry if this has already been resolved.] > > Jozef Lawrynowicz <jozef.l@mittosystems.com> writes: > > For MSP430, the folding of identical functions marked with the "interrupt" > > attribute by -fipa-icf-functions results in wrong code being generated. > > Interrupts have different calling conventions than regular functions, so > > inserting a CALL from one identical interrupt to another is not correct and > > will result in stack corruption. > > > > I imagine there are other targets that also have different calling conventions > > for interrupt functions compared to regular functions, and so folding them > > would be undesirable. > > > > Therefore, I would appreciate some feedback as to whether it would be welcomed > > to fix this in a generic way or if I should just keep it MSP430 specific. > > > > 1. MSP430 specific > > Add the "no_icf" attribute to functions marked with the "interrupt" attribute > > when processing them in the backend. > > > > 2. Target Hook > > Add a DEFHOOKPOD (e.g. TARGET_NO_INTERRUPT_ICF) which controls whether ICF is > > disabled for functions with the interrupt attribute (in gcc/ipa-icf.c, where > > "no_icf" attribute is processed). > > This sounds like it should be a question about two functions rather > than one, i.e.: can functions A and B be merged together if they > appear to be identical at some level? I think for most targets, > merging two identical interrupt functions would be OK. It's merging > an interrupt and non-interrupt function that's the problem. > > So maybe we need something like targetm.can_inline_p, but for > merging rather than inlining? The function would need to be > transitive of course. > > Thanks, > Richard By "merging" are you referring to aliasing i.e. only keep one copy of the function and make the addresses of the functions equal? The problem for me is that the properties which prevent an alias being created for functions in ipa-icf.c, could be equally applicable to interrupt functions. At least for MSP430, there is no specific assertion that we can disregard the addresses of interrupt functions. It is feasible that a user might want to compare the addresses of different interrupt functions and they would expect those addresses to be different. In ipa-icf.c we have: > /* Creating a symtab alias is the optimal way to merge. > It however can not be used in the following cases: > 1) if ORIGINAL and ALIAS may be possibly compared for address equality. > .... It makes this decision about whether ORIGINAL and ALIAS might be compared for address equality by looking at if the addresses "matter". I'm simplifying here, but essentially a function address is then decided to "matter" if it is externally visible, unless it is a virtual table/function or C++ ctor/dtor. If we had this targetm.can_alias_p hook evaluated as part of the exceptions to whether an address matters, I don't think for MSP430 we could safely make it true for interrupts. Thanks, Jozef ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Disabling ICF for interrupt functions 2019-07-30 13:41 ` Jozef Lawrynowicz @ 2019-07-31 9:50 ` Richard Biener 2019-07-31 12:22 ` Jozef Lawrynowicz 0 siblings, 1 reply; 8+ messages in thread From: Richard Biener @ 2019-07-31 9:50 UTC (permalink / raw) To: Jozef Lawrynowicz; +Cc: Richard Sandiford, GCC Development, Alexander Monakov On Tue, Jul 30, 2019 at 3:41 PM Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote: > > On Fri, 26 Jul 2019 18:39:50 +0100 > Richard Sandiford <richard.sandiford@arm.com> wrote: > > > [Catching up after being away, sorry if this has already been resolved.] > > > > Jozef Lawrynowicz <jozef.l@mittosystems.com> writes: > > > For MSP430, the folding of identical functions marked with the "interrupt" > > > attribute by -fipa-icf-functions results in wrong code being generated. > > > Interrupts have different calling conventions than regular functions, so > > > inserting a CALL from one identical interrupt to another is not correct and > > > will result in stack corruption. > > > > > > I imagine there are other targets that also have different calling conventions > > > for interrupt functions compared to regular functions, and so folding them > > > would be undesirable. > > > > > > Therefore, I would appreciate some feedback as to whether it would be welcomed > > > to fix this in a generic way or if I should just keep it MSP430 specific. > > > > > > 1. MSP430 specific > > > Add the "no_icf" attribute to functions marked with the "interrupt" attribute > > > when processing them in the backend. > > > > > > 2. Target Hook > > > Add a DEFHOOKPOD (e.g. TARGET_NO_INTERRUPT_ICF) which controls whether ICF is > > > disabled for functions with the interrupt attribute (in gcc/ipa-icf.c, where > > > "no_icf" attribute is processed). > > > > This sounds like it should be a question about two functions rather > > than one, i.e.: can functions A and B be merged together if they > > appear to be identical at some level? I think for most targets, > > merging two identical interrupt functions would be OK. It's merging > > an interrupt and non-interrupt function that's the problem. > > > > So maybe we need something like targetm.can_inline_p, but for > > merging rather than inlining? The function would need to be > > transitive of course. > > > > Thanks, > > Richard > > By "merging" are you referring to aliasing i.e. only keep one copy of the > function and make the addresses of the functions equal? > > The problem for me is that the properties which prevent an alias being created > for functions in ipa-icf.c, could be equally applicable to interrupt functions. > At least for MSP430, there is no specific assertion that we can disregard the > addresses of interrupt functions. It is feasible that a user might want to > compare the addresses of different interrupt functions and they would expect > those addresses to be different. But those can the use -fno-ipa-icf. Wouldn't users prefer smaller binaries by merging identical interrupt handlers as well? Richard. > > In ipa-icf.c we have: > > /* Creating a symtab alias is the optimal way to merge. > > It however can not be used in the following cases: > > 1) if ORIGINAL and ALIAS may be possibly compared for address equality. > > .... > > It makes this decision about whether ORIGINAL and ALIAS might be compared for > address equality by looking at if the addresses "matter". > I'm simplifying here, but essentially a function address is then decided to > "matter" if it is externally visible, unless it is a virtual table/function or > C++ ctor/dtor. > If we had this targetm.can_alias_p hook evaluated as part of the exceptions to > whether an address matters, I don't think for MSP430 we could safely make it > true for interrupts. > > Thanks, > Jozef ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] Disabling ICF for interrupt functions 2019-07-31 9:50 ` Richard Biener @ 2019-07-31 12:22 ` Jozef Lawrynowicz 0 siblings, 0 replies; 8+ messages in thread From: Jozef Lawrynowicz @ 2019-07-31 12:22 UTC (permalink / raw) To: Richard Biener; +Cc: Richard Sandiford, GCC Development, Alexander Monakov On Wed, 31 Jul 2019 11:49:58 +0200 Richard Biener <richard.guenther@gmail.com> wrote: > On Tue, Jul 30, 2019 at 3:41 PM Jozef Lawrynowicz > <jozef.l@mittosystems.com> wrote: > > > > On Fri, 26 Jul 2019 18:39:50 +0100 > > Richard Sandiford <richard.sandiford@arm.com> wrote: > > > > > [Catching up after being away, sorry if this has already been resolved.] > > > > > > Jozef Lawrynowicz <jozef.l@mittosystems.com> writes: > > > > For MSP430, the folding of identical functions marked with the "interrupt" > > > > attribute by -fipa-icf-functions results in wrong code being generated. > > > > Interrupts have different calling conventions than regular functions, so > > > > inserting a CALL from one identical interrupt to another is not correct and > > > > will result in stack corruption. > > > > > > > > I imagine there are other targets that also have different calling conventions > > > > for interrupt functions compared to regular functions, and so folding them > > > > would be undesirable. > > > > > > > > Therefore, I would appreciate some feedback as to whether it would be welcomed > > > > to fix this in a generic way or if I should just keep it MSP430 specific. > > > > > > > > 1. MSP430 specific > > > > Add the "no_icf" attribute to functions marked with the "interrupt" attribute > > > > when processing them in the backend. > > > > > > > > 2. Target Hook > > > > Add a DEFHOOKPOD (e.g. TARGET_NO_INTERRUPT_ICF) which controls whether ICF is > > > > disabled for functions with the interrupt attribute (in gcc/ipa-icf.c, where > > > > "no_icf" attribute is processed). > > > > > > This sounds like it should be a question about two functions rather > > > than one, i.e.: can functions A and B be merged together if they > > > appear to be identical at some level? I think for most targets, > > > merging two identical interrupt functions would be OK. It's merging > > > an interrupt and non-interrupt function that's the problem. > > > > > > So maybe we need something like targetm.can_inline_p, but for > > > merging rather than inlining? The function would need to be > > > transitive of course. > > > > > > Thanks, > > > Richard > > > > By "merging" are you referring to aliasing i.e. only keep one copy of the > > function and make the addresses of the functions equal? > > > > The problem for me is that the properties which prevent an alias being created > > for functions in ipa-icf.c, could be equally applicable to interrupt functions. > > At least for MSP430, there is no specific assertion that we can disregard the > > addresses of interrupt functions. It is feasible that a user might want to > > compare the addresses of different interrupt functions and they would expect > > those addresses to be different. > > But those can the use -fno-ipa-icf. > > Wouldn't users prefer smaller binaries by merging identical interrupt handlers > as well? > > Richard. > It seems I omitted some key information from my OP which could be why there is some disagreement here. Originally, I was referring to interrupt handlers which DO NOT have a vector specified. i.e. "__attribute__((interrupt))" I still think it is undesirable to alias different interrupt handlers, with identical contents, which do not specify a vector. When there is no vector specified, the user is probably remapping these handlers at link-time or run-time, possibly into different vectors. Even though these handlers are not directly callable, their addresses should still be considered to "matter" (like a regular globally visible function would) and so they shouldn't be aliased. I would agree that interrupt handlers which have the same vector specified and have identical contents should be aliased. They will be mapped to the same address anyway so there really will be wasted space in the final binary. GCC currently won't merge identical interrupt handlers with different vectors specified in the attribute. I think this is ok. Thanks, Jozef ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-07-31 12:22 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-19 12:45 [RFC] Disabling ICF for interrupt functions Jozef Lawrynowicz 2019-07-19 13:32 ` Alexander Monakov 2019-07-22 17:01 ` Jozef Lawrynowicz 2019-07-22 18:50 ` Alexander Monakov 2019-07-26 17:39 ` Richard Sandiford 2019-07-30 13:41 ` Jozef Lawrynowicz 2019-07-31 9:50 ` Richard Biener 2019-07-31 12:22 ` Jozef Lawrynowicz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).