[PATCH] D83635: [OpenMPOpt][WIP] Merge parallel regions

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

It's great that you're working on this. It's very important that we allow people to write code, structured and decomposed in a way that makes sense from an engineering and maintenance perspective, and have the compiler combine things later to avoid unnecessary overhead. This is just as much true for expressions of parallelism as it is for other aspects of the code.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:744
+      // Merge maximal number of succesive CIs when in-between
+      // instructions are safe to execute in parallel
+      for (Instruction &I : *BB) {
----------------
Comments in LLVM should be complete sentences and end with appropriate punctuation (here and a few other places).


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:751
+
+        if (isSafeToSpeculativelyExecute(&I, &I, DT)) {
+          continue;
----------------
Uneeded {} (here and a few other places).


================
Comment at: llvm/test/Transforms/OpenMP/parallel_region_merging.ll:13
+;    void merge_all() {
+;    #pragma omp parallel
+;        {
----------------
Do these need to say 'nowait' in order to actually match the code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83635