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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Thanks for the update! With the second analysis we can show why the frontend is not necessarily the perfect place for this, determining `readonly` happens naturally in the middle-end. (Not to mention with the analysis for sufficient conditions to know a transformation would be sound.).

I left a bunch of comments, nothing major though.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:158
+    Changed |= analysisConstantThreadNum();
+    Changed |= analysisConstantShared();
 
----------------
I doubt these need to return a value.

Can we check if remarks are enabled and only run them in case they are?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:342
                       << ore::NV("OpenMPRuntimeMoves", newLoc->getDebugLoc());
           };
           emitRemark<OptimizationRemark>(CI, "OpenMPRuntimeCodeMotion", Remark);
----------------
Why an X-value (`&&`) and not just a reference (`&`)?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:458
+    return false;
+  }
+
----------------
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.)




================
Comment at: llvm/test/Transforms/OpenMP/shared_firstprivate_analysis.ll:13
+
+; CHECK: remark: shared_firstprivate_analysis.c:7:5: Use of OpenMP shared clause with a value that should be pass-by-value. Consider using the firstprivate caluse instead
+define dso_local void @shared(i32 %0) local_unnamed_addr #0 !dbg !14 {
----------------
We need to reference the value somehow. Where does this point to now, I mean what is in line 7? Could we include the C source in a comment at the top? We probably want to issue 2 remarks, one for the value, with the value debug info, and one for the directive.


================
Comment at: llvm/test/Transforms/OpenMP/shared_firstprivate_analysis.ll:125
+!77 = distinct !DILexicalBlock(scope: !68, file: !1, line: 13, column: 5)
+!78 = !DILocation(line: 13, column: 5, scope: !58)
----------------
jhuber6 wrote:
> Should I combine this into one giant file that just handles OpenMP Analysis stuff?
If there is no inherent benefit I would not merge them. Having more selective tests is usually better. You can prefix their names to make it easier to categorize them (instead of the current postfix), or put them into a subfolder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81036