This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Split PowerPC-specific code out of IntrinsicCall into PPCIntrinsicCall
ClosedPublic

Authored by pscoro on Jun 8 2023, 10:37 AM.

Details

Summary

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.

Diff Detail

Event Timeline

pscoro created this revision.Jun 8 2023, 10:37 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
pscoro requested review of this revision.Jun 8 2023, 10:37 AM
pscoro edited the summary of this revision. (Show Details)Jun 8 2023, 10:42 AM
sunshaoce added inline comments.Jun 12 2023, 6:06 AM
flang/include/flang/Optimizer/Builder/PPCIntrinsicCall.h
2

Maybe //==-- Builder/PPCIntrinsicCall.h - lowering of PowerPC intrinsics -*-C++-*-==// is better?

pscoro updated this revision to Diff 530487.Jun 12 2023, 6:29 AM

Nit: add c++ to PPCIntrinsicCall header comment

pscoro marked an inline comment as done.Jun 12 2023, 6:30 AM
jeanPerier added inline comments.Jun 13 2023, 7:06 AM
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.
It will also force recompilation of the files including these file every time an intrinsic is added and might increase their compilation time to process the constexpr table.

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.

pscoro updated this revision to Diff 531340.Jun 14 2023, 8:07 AM

Different approach: moved handlers and mathOperation to cpp files

pscoro added inline comments.Jun 14 2023, 8:10 AM
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

jeanPerier accepted this revision.Jun 15 2023, 8:48 AM

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).

This revision is now accepted and ready to land.Jun 15 2023, 8:48 AM
pscoro added inline comments.Jun 15 2023, 11:46 AM
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.

pscoro updated this revision to Diff 532184.Jun 16 2023, 9:02 AM

Rebase, recent PPC changes moved to new PPCIntrinsicCall as well