On Tue, Jul 30, 2019 at 3:41 PM Jozef Lawrynowicz <joze...@mittosystems.com> wrote: > > On Fri, 26 Jul 2019 18:39:50 +0100 > Richard Sandiford <richard.sandif...@arm.com> wrote: > > > [Catching up after being away, sorry if this has already been resolved.] > > > > Jozef Lawrynowicz <joze...@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
