[PATCH] D79359: OpenMPOpt Remarks Support

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Please diff against the base version, e.g., by squashing all your commits into a single one.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:511
+        [&]() { return RemarkCB(RemarkKind(DEBUG_TYPE, RemarkName, Inst)); });
+  }
+
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > Nit: getFunction()
> > 
> > auto & or auto *, please.
> > 
> > Do you need a default for the callback type?
> 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.
The latter. Doesn't this work:
```
template <typename RemarkKind, typename RemarkCallBack>
```


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:612
+      return std::make_unique<OptimizationRemarkEmitter>(F);
+    };
+
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > Call getAnalysis here?
> 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.
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79359/new/

https://reviews.llvm.org/D79359