This patch moves PPC intrinsic generator code to PPCIntrinsicCall.cpp. In order to move PowerPC intrinsic code out of IntrinsicCall.cpp, we need to also move some declarations to IntrinsicCall.h. handlers[] and mathOperations[] were also chosen to be moved to the IntrinsicCall header. Similarly, ppcHandlers[] and ppcMathOperations[] were moved to the PPCIntrinsicCall header. There are future patches coming up that will introduce many new PPC intrinsics, these will now be defined in PPCIntrinsicCall.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
flang/include/flang/Optimizer/Builder/PPCIntrinsicCall.h | ||
---|---|---|
1 | Maybe //==-- Builder/PPCIntrinsicCall.h - lowering of PowerPC intrinsics -*-C++-*-==// is better? |
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
599 | What is the motivation to move this in the header? In my opinion this makes the API that is intended to be usable from outside harder to understand. So if their is no strong motivation to move them, I would rather keep the tables in the .cpp and minimize the cost of including these headers/make them. |
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
599 | Based on your comments I have tried a different approach. handlers[] and mathOperations[] are back in IntrinsicCall.cpp. ppcHandlers[] and ppcMathOperations[] are moved to PPCIntrinsicCall.cpp for consistency. The way IntrinsicCall.cpp accesses ppcHandlers required no changes to support this, but the way it accessed ppcMathOperations did. There is a new function checkPPCMathOperationsRange that is used to check for a name in ppcMathOperations |
Looks good, just bringing something to your attention in case you were not aware.
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
564 | Note the usage of these template functions in the ppc file are working a bit out of luck because all the instantiation you use in the ppc file are also instantiated in IntrinsicCall.cpp that has the implementation for these template functions. Otherwise, you would likely hit linking issues. If you ever need to use it for operations not used in IntrinsicCall.cpp, you will need to explicitly force template instantiation IntrinsiCall.cpp, or share the implementation in the header (maybe with the builder.create<T>(loc, args) being done in the header as a callback to erase the template from the implementation in the .cpp). |
flang/include/flang/Optimizer/Builder/IntrinsicCall.h | ||
---|---|---|
564 | Will keep this in mind. We had similar issues with explicit instantiations in previous drafts of this patch because of the templating on our PPCIntrinsicLibrary's intrinsic generators (eg genMtfsf), we fixed this by moving all the code that searches the ppcHandlers/ppcMathOps tables into PPCIntrinsicCall.cpp as well. If my understanding is correct, we avoided running into a similar issue here because, although we have function pointers to genMathOp in PPCIntrinsicCall.cpp, because the invocations always only happen in IntrinsicCall.cpp, there is no need to explicitly instantiate in PPCIntrinsicCall.cpp? We plan on continuing to keep alot of the core functionality (like the generator invocations) to IntrinsicCall.cpp so i dont believe we will run into this issue, however if we do, I believe we can address it when it occurs. |
Note the usage of these template functions in the ppc file are working a bit out of luck because all the instantiation you use in the ppc file are also instantiated in IntrinsicCall.cpp that has the implementation for these template functions. Otherwise, you would likely hit linking issues.
If you ever need to use it for operations not used in IntrinsicCall.cpp, you will need to explicitly force template instantiation IntrinsiCall.cpp, or share the implementation in the header (maybe with the builder.create<T>(loc, args) being done in the header as a callback to erase the template from the implementation in the .cpp).