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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:342
                       << ore::NV("OpenMPRuntimeMoves", newLoc->getDebugLoc());
           };
           emitRemark<OptimizationRemark>(CI, "OpenMPRuntimeCodeMotion", Remark);
----------------
jhuber6 wrote:
> 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.
It's fine, I didn't check this deep, just from the diff I figured I ask.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:147
 
+  bool remarksEnabled() {
+    auto &Ctx = M.getContext();
----------------
Nit: Documentation, also all the functions below please.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:405
+        }
+      }
+      return nullptr;
----------------
Style: I'd get rid of the braces, makes it (for me) harder to argue if the return below is inside the lambda or not. 


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:415
+    } else {
+      return {};
+    }
----------------
I think the convention is `llvm::None`, or something similar. Above, just `return upper...` so we can also remove the braces. While at it, remove the else (no else after return).


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:420
+  Function *getParallelRegionEntry(CallInst *CI) {
+    auto *ParallelRegionFn = &CI->getOperandUse(2);
+    AbstractCallSite ACS(ParallelRegionFn);
----------------
Somewhere in here we did:
```
    const unsigned CallbackCalleeOperand = 2;
```
To give the magic number 2 a description. Maybe move it into class scope or duplicate it.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:444
+                         << "Consider using a scalable thread count, "
+                         << "or remove the 'num_threads' clause.";
+            };
----------------
I think we should copy the pointer `C` by value here, `[=]` or `[C]`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81036