[PATCH] D75384: OpenMP for loop fusion

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

You need to squash your commits or create the diff against the proper base commit. More comments inlined but there is a lot more to comment on.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:151
+  ///  The __kmpc_static_fini() of Loop-1 and __kmpc_static_init4() of Loop-2 
+  /// can be removed and two loops can be run under a single pair of __kmpc_static_init4() and __kmpc_static_fini() calls
+
----------------
The example above is generally nice but maybe we should not print values in parallel, just initialize two arrays.

Style: Clang format all code and comments (=the entire file). Also adjust the indention in the example.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:153
+
+  /// The following function is the main pipeline for  the whole process
+  bool deleteStaticScheduleCalls() {
----------------
Nit: two spaces, if you write sentences add the '.'.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:166
+        /// The following replaces  the use values in the combined  loop
+      if (Changed) replace_UseValues(*F, &OLF);
     }
----------------
Changed should never be set to false once it is true. However, only because you do that this works. You need a local changed or just `if(...)` somewhere.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:178
     return false;
-  }
+ }
 
----------------
Having a function like this is often a side-effect of improper data-structures. If you need to cache all calls that have been looked at, add a `SmallPtrSet<CallInst*, 8>` to do so instead of iterating over all vectors that are keys in a map.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:191
       std::vector<CallInst *> v;
-      if (find(itr->first, OLF->call_map))
-        continue;
-      for (auto itr1 = itr; itr1 != OLF->call_init_fini_mapping.end(); ++itr1) {
-        if (itr == itr1)
-          continue;
-        else {
-          std::vector<Value *> v1, v2;
-          for (Value *arg : (itr->first)->args()) {
+      std::vector<Value *> v1;
+        /// if not then store the arguments of __kmpc_static_init4 in vector v1
----------------
Please use llvm data structures and give variables proper names.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:228
     }
-    return false;
   }
 
----------------
This function is quadratic in the number of `__kmpc_static_init4` calls.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:250
     }
-  }
+ }
+
----------------
This class provides a way to access/iterate over all call sites of a known OpenMP runtime call. Please use that.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:309
+      		r->eraseFromParent();
+}
+/// does function printing
----------------
The formatting of this function makes it really hard to read it. Please clang-format your patch before uploading it.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:309
+      		r->eraseFromParent();
+}
+/// does function printing
----------------
jdoerfert wrote:
> The formatting of this function makes it really hard to read it. Please clang-format your patch before uploading it.
> 
Please do not iterate over the entire function, basically ever. Start with the things you are interested in and go from there. Iterating over the entire function is at some point costly and always wasteful.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:313
+//    F.print(errs(), nullptr);
+//  }
 
----------------
No commented out code.


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

https://reviews.llvm.org/D75384