[PATCH] D79359: OpenMPOpt Remarks Support

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 marked 3 inline comments as done.
jhuber6 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:511
+        [&]() { return RemarkCB(RemarkKind(DEBUG_TYPE, RemarkName, Inst)); });
+  }
+
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:560
+    return std::make_unique<OptimizationRemarkEmitter>(std::move(ORE));
+  };
+
----------------
jdoerfert wrote:
> I don't think it's a good idea to move the result here. Just return a reference. 
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79359