[PATCH] D75384: OpenMP for loop fusion

[Abid via Phabricator via llvm-commits]

abidmalikwaterloo marked 8 inline comments as done.
abidmalikwaterloo added a comment.

See the new patch

https://reviews.llvm.org/D90103

I have submitted the new patch  As I could not upload the modifications through this thread.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:178
     return false;
-  }
+ }
 
----------------
jdoerfert wrote:
> 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.
See the new patch
https://reviews.llvm.org/D90103


================
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
----------------
jdoerfert wrote:
> Please use llvm data structures and give variables proper names.
See the new patch

https://reviews.llvm.org/D90103


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

https://reviews.llvm.org/D90103


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

https://reviews.llvm.org/D90103

I removed this function in the new implementation.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:309
+      		r->eraseFromParent();
+}
+/// does function printing
----------------
jdoerfert wrote:
> 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.
See the new patch

https://reviews.llvm.org/D90103

I removed this in the new patch.


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

https://reviews.llvm.org/D75384