[PATCH] D81036: [OpenMP] Begin Adding Analysis Remarks for OpenMP Best Practises.

[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:158
+    Changed |= analysisConstantThreadNum();
+    Changed |= analysisConstantShared();
 
----------------
jdoerfert wrote:
> I doubt these need to return a value.
> 
> Can we check if remarks are enabled and only run them in case they are?
I think there's an option for checking in here but I haven't tried yet, 
[[ https://llvm.org/doxygen/classllvm_1_1DiagnosticInfoOptimizationBase.html | https://llvm.org/doxygen/classllvm_1_1DiagnosticInfoOptimizationBase.html ]]


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:342
                       << ore::NV("OpenMPRuntimeMoves", newLoc->getDebugLoc());
           };
           emitRemark<OptimizationRemark>(CI, "OpenMPRuntimeCodeMotion", Remark);
----------------
jdoerfert wrote:
> Why an X-value (`&&`) and not just a reference (`&`)?
As far as I know, the Remark is being generated here as an r-value so the callback function can either copy it or move it, but can't take a reference to it since it has no location in memory. An r-value could bind to a const reference but the emitter needs to be mutated to build the remark.
```
 ORE.emit([&]() {
   return RemarkCB(RemarkKind(DEBUG_TYPE, RemarkName, Inst));
 });
```
I could change this to generate a RemarkKind inside the function and allow a generic reference, but it would pretty much just change whether or not the object is constructed here or when emit calls the lambda function. That's at least my understanding, I'm not an r-value l-value guru.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:458
+    return false;
+  }
+
----------------
jdoerfert wrote:
> Nit: -`;`
> Hoist `RFI` out of the loop.
> Don't we have a helper to do something "foreach call"... we should.
> 
> I would not call it `Callback` but `ParallelRegionFn` or similar. Also use proper types not auto when it is not clear and helpful, e.g., for `Callback`.
> 
> I'd add the word "probably" or "very likely" in the message as it is only known to be the case if the argument also is not modified via a different pointer. (For example, only if it is `noalias` as well we know it is not otherwise modified and can do the transformation ourselves.)
> 
> 
There's the "foreachUse" function but I wasn't sure if it applied here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81036