[PATCH] D79359: OpenMPOpt Remarks Support

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Last round of comments. After we should be good to go.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:511
+        [&]() { return RemarkCB(RemarkKind(DEBUG_TYPE, RemarkName, Inst)); });
+  }
+
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > 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>
> > ```
> 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?
I don't get the last comment but I guess we can just leave it as is.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:30
 
+#include <memory>
+
----------------
Nit: probably not needed.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:63
+            CallGraphUpdater &CGUpdater,
+            OptimizationRemarkGetter &&OREGetter)
       : M(*(*SCC.begin())->getParent()), SCC(SCC), ModuleSlice(ModuleSlice),
----------------
Just copy the getter (=function_ref)


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:616
+              std::make_unique<OptimizationRemarkEmitter>(F)));
+      return *OREMap[F];
+    };
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79359