Re: [RFC] Disabling ICF for interrupt functions

[Jozef Lawrynowicz <jozef.l@mittosystems.com>]

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