[PATCH] D75384: OpenMP for loop fusion

[Abid via Phabricator via llvm-commits]

abidmalikwaterloo marked 7 inline comments as done and an inline comment as not done.
abidmalikwaterloo added a comment.
Herald added a subscriber: yaxunl.

In D75384#1932847 <https://reviews.llvm.org/D75384#1932847>, @jdoerfert wrote:

> This doesn't seem to use dominance at all. How do you handle
>
>   if (a) {
>   #pragma omp for
>   for (int i = 0; i < 10; i++)
>     ;
>   } else {
>   #pragma omp for
>   for (int i = 0; i < 10; i++)
>     ;
>   }
>


Based on. the value of a, we will only have only one "for loop". Therefore, for this specific case,  the implemented technique  will see only one "for loop" at the IR level. This case should be a problem.



================
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
+
----------------
jdoerfert wrote:
> 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.
done


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:153
+
+  /// The following function is the main pipeline for  the whole process
+  bool deleteStaticScheduleCalls() {
----------------
jdoerfert wrote:
> Nit: two spaces, if you write sentences add the '.'.
not sure what does this means.   I should add ".. at the end of each comment line????


================
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);
     }
----------------
jdoerfert wrote:
> 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.
**Changed **variable is confused with the **Changed** variable within the run() function.  Changed it to local "changed=false"


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


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

https://reviews.llvm.org/D75384