Adding initial support for emitting remarks inside of OpenMPOpt. Added a Callback function to get an OptimizationRemarkEmitter object to the OpenMPOpt struct. The callback is used by the emitRemark function. Added two locations that emit simple remarks and a test file to verify functionality.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | ||
---|---|---|
560 | I changed the OREGetter to return a reference. I'm not exactly sure what you mean by default for the callback type. Do you mean a default implementation or are you talking about just the template parameter. | |
613 | Yeah, I did this initially because I was having issues getting both the legacy and new versions to return a reference without memory leaks so I just did it by returning a unique_ptr. The std::move just cast it to an rvalue to avoid using the deleted constructor in the ORE class. I found another file that did something similar that had a single unique_ptr object to an ORE that's captured by reference. Then you just return a reference to that, so I'll change it to use that method because it's probably much more efficient. | |
667 | I tried using the getAnalysis method but it caused it to seg-fault. I'm not exactly confident why but some other passes generate the ORE this way so I figured it was okay. |
Please diff against the base version, e.g., by squashing all your commits into a single one.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | ||
---|---|---|
560 | The latter. Doesn't this work: template <typename RemarkKind, typename RemarkCallBack> | |
667 | Oh, right. Yeah, so, the old PM CGSCC pass is bad at requiring function passes. Long story short, it doesn't work. We should do what the inliner does, create an ORE whenever we need one for a function. Since we would also like hotness information in the new pass manager (which I think we should get automatically via the pass), we should not always create a new ORE but only in the old PM. That said, the ORE constructor says it can be expensive if hotness filtering is requested as block frequency is computed. Thus, in the old PM we can cache the OREs ourselves. So, we keep a map from function to ORE and in the OREGetter we lookup the ORE, if not there create it, and return it. WDYT? |
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | ||
---|---|---|
560 | I was just using that template to rename the type inside the function. I was pretty much just using it as this: using RemarkCallBack = function_ref<RemarkKind(RemarkKind &&)>>; Should I change that? | |
667 | I was thinking about adding a cache previously since the function being worked on might not change often. It might be somewhat difficult to bake that functionality into the callback but I'll see what I can do in the morning. |
I added a DenseMap to cache the Functions for the legacy PM's getter function. Let me know if I should've used a different data structure for this.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | ||
---|---|---|
667 | I just noticed I could probably have used try_emplace() rather than checking and inserting manually. |
Last round of comments. After we should be good to go.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | ||
---|---|---|
29 | Nit: probably not needed. | |
60 | Just copy the getter (=function_ref) | |
560 | I don't get the last comment but I guess we can just leave it as is. | |
671 | I usually prefer: std::make_unique<OptimizationRemarkEmitter> &ORE = OREMap[F]; if(!ORE) ORE = ... return ORE; Your code has 3 lookups into the map, mine only one. |
Adding a remark and test for parallel region deletion. Added a NV tag for the runtime function deduplicated / moved.
Just realized the master branch changed the attributor flags for the test file. Should I add a diff that's more up-to-date?
Nit: probably not needed.