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. > > -ChrisAgree. 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. >
Alireza.Moshtaghi at microchip.com
2009-Aug-24 19:12 UTC
[LLVMdev] ISRs for PIC16 [was [llvm]r79631 ...]
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 butthere> >> 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 isbest> >> 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 sectionstring> > for the lack of a better attribute. > > > > Plus we also wanted to encode some information to inform codegenabout> > whether we are codegenerating an interrupt line function. Again the > > section string is used to encode the same. Probably annotationsmight be> > a better place but that does not work currently (and also, accessing > > them slower). This information is required so that the codegen canemit> > calls to appropriate intrinsics. > > > > So, IMO, the approach is simple, but probably the implementationwasn't> > clean. > > > > Ideas welcome. > >