llvm dev - Aug 2009 - [LLVMdev] ISRs for PIC16 [was [llvm]r79631 ...]

Chris Lattner wrote:
>
> On Aug 21, 2009, at 11:13 AM, Alireza.Moshtaghi at microchip.com wrote:
>
>>>> Add a pass to do call graph analyis to overlay the autos and
frame
>>>> sections of
>>>> leaf functions. This pass will be extended to color other nodes
of
>>>> the call tree
>>>> as well in future.
>>>
>>> Sanjiv, this commit, r79562 and r79563 are not the right way to
tackle
>>> this problem, and are coming in the day of the release branch. 
Please
>>> revert them and propose patches for these on llvmdev.
>>
>> Hi Chris
>>
>> The reason why we are doing these in backend is that cloning a function
>> is base on which functions call it and of course in frontend we
don't
>> have the view of the entire call graph. We'll be happy to revert
these,
>> but I would like to know exactly why in your view.
>
> We should discuss this on llvmdev, I think it came up before but there 
> was no conclusive plan that was proposed.
The approach that we thought to for for PIC16 can be described in a 
single line as below.

"Keep the functions called from ISR and main separate by cloning the  
shared ones".
>
> In short, I think that this sort of thing should be modeled as an 
> attribute on function definitions, not declarations.  I don't 
> understand why you need to clone entire call graphs, but this is best 
> discussed on llvmdev.
It isn't cloning the entire call graph. It's cloning only the shared 
ones as mentioned above.

The information that a function is ISR is encoded in the section string 
for the lack of a better attribute.

Plus we also wanted to encode some information to inform codegen about 
whether we are codegenerating an interrupt line function. Again the 
section string is used to encode the same. Probably annotations might be 
a better place but that does not work currently (and also, accessing 
them slower). This information is required so that the codegen can emit 
calls to appropriate intrinsics.

So, IMO, the approach is simple, but probably the implementation wasn't 
clean.

Ideas welcome.
>
> Trying to cram things into 2.6 at the last minute is not acceptable. 
> :)  For major things like this, it should have a real discussion about 
> the design and the way forward and the implementation should have 
> landed more than the day of the branch.
>
> -Chris
Agree. That was't a good thing on our part.

- Sanjiv

Bringing it up again.

- Sanjiv
Sanjiv Gupta wrote:
> Chris Lattner wrote:
>   
>> We should discuss this on llvmdev, I think it came up before but there 
>> was no conclusive plan that was proposed.
>>     
> The approach that we thought for PIC16 can be described in a 
> single line as below.
>
> "Keep the functions called from ISR and main separate by cloning the  
> shared ones".
>
>   
>> In short, I think that this sort of thing should be modeled as an 
>> attribute on function definitions, not declarations.  I don't 
>> understand why you need to clone entire call graphs, but this is best 
>> discussed on llvmdev.
>>     
> It isn't cloning the entire call graph. It's cloning only the
shared
> ones as mentioned above.
>
> The information that a function is ISR is encoded in the section string 
> for the lack of a better attribute.
>
> Plus we also wanted to encode some information to inform codegen about 
> whether we are codegenerating an interrupt line function. Again the 
> section string is used to encode the same. Probably annotations might be 
> a better place but that does not work currently (and also, accessing 
> them slower). This information is required so that the codegen can emit 
> calls to appropriate intrinsics.
>
> So, IMO, the approach is simple, but probably the implementation wasn't
> clean.
>
> Ideas welcome.
>

Up front I apologize for the lengthy email. Since our patch was not
accepted, Chris asked us to follow up with this issue on llvm-dev. For
the sake of completeness, let me give a bit of background and the
problems that we are facing. I tried to capture as much as possible here
so Please do give us feedback. 
Don't take your stack for granted.... PIC16 has very poor pointer
handling, hence stack access are very limited and inefficient; so we do
static analysis of function frame size and statically allocate the
function frame in memory (including locals, arguments and spills); then,
to save memory, we overlay the frame of functions that are not on the
same call branch. 
Consequently, functions are non reentrant. We can sacrifice recursion
(because of other limitations in pic16 architecture) but still
reentrancy is needed because ISR thread can call same function that is
also called from main thread; examples are stdlib functions and
intrinsic for math (div/mul/float/etc)
Here is what we did - but was rejected and we would like to know which
part (if any) we can keep and which part we must change:
1- Root ISR function is defined by user. Since the ISR is located at
certain address in program memory, we used the section attribute of
function to encode that the function needs to be at a section with fixed
address (our in-house linker takes care of the details). We did not add
new interrupt attribute because it seemed redundant because we can
encode whatever is needed in the section attribute in the front-end.
2- Added two passes at the end of llvm-ld. First pass does call graph
analysis to distinguish ISR thread from main thread and add few more
pieces of info to section attribute of the function to indicate whether
it is called from main thread or ISR thread; if called from both, the
function has to be cloned. To clone, code and data must be cloned. Not
having stack, and accessing local variables through their symbolic
reference, we need to also clone the local variables and modify the
cloned function to use different naming convention for local variables
(there are other ways to do this also but in practice, that requires two
different frameIndexes for one function one for locals and the other for
args and spills). So after this pass, the main thread and ISR thread are
completely disjoint; this takes care of stdlib functions as well because
they are defined in .bc or .a library files (there is a caveat on
function pointers that I'm going to explain later). The second pass does
simple function coloring to identify which function frames can be
overlaid. Color info is also added to section attribute.
3- Code generation is mostly similar for main vs ISR threads except for
function interface of root ISR; but the most important problem is the
intrinsic functions for non-native operations such as mul/div and
floating point. So while generating code for these calls, we use
different naming conventions depending on whether we are generating code
for a function on ISR-thread or main-thread (this information is in the
section attribute of the function). Two versions of intrinsics exist in
the pic16 specific library and the linker will use according to naming
convention.
4- There is a bit of cleanup left to be done in AsmPrinter and we are
good to go.
5- Handling of function pointers is not implemented yet. But since we
don't have dynamic linking, through alias analysis we should be able to
find targets of each pointer and when taking address of these functions,
depending on whether it is in isr or main thread, we can resolve the
problem efficiently. This can be done in a separate pass; the only thing
that we have to disallow is calling same function pointer from both the
ISR and main threads.
6- Handling of varargs requires that a separate frame be created for
arguments for each call site with varargs. This is going to be handled
while codegen and by the overlay algorithm to save memory.

If you have come this far, thank you for being interested in my brain
dysfunction. And I am really interested to know your input.

-Ali

> >> We should discuss this on llvmdev, I think it came up before but
there
> >> was no conclusive plan that was proposed.
> >>
> > The approach that we thought for PIC16 can be described in a
> > single line as below.
> >
> > "Keep the functions called from ISR and main separate by cloning
the
> > shared ones".
> >
> >
> >> In short, I think that this sort of thing should be modeled as an
> >> attribute on function definitions, not declarations.  I don't
> >> understand why you need to clone entire call graphs, but this is
best
> >> discussed on llvmdev.
> >>
> > It isn't cloning the entire call graph. It's cloning only the
shared
> > ones as mentioned above.
> >
> > The information that a function is ISR is encoded in the section
string
> > for the lack of a better attribute.
> >
> > Plus we also wanted to encode some information to inform codegen
about
> > whether we are codegenerating an interrupt line function. Again the
> > section string is used to encode the same. Probably annotations
might be
> > a better place but that does not work currently (and also, accessing
> > them slower). This information is required so that the codegen can
emit
> > calls to appropriate intrinsics.
> >
> > So, IMO, the approach is simple, but probably the implementation
wasn't
> > clean.
> >
> > Ideas welcome.
> >