[PATCH] D75384: OpenMP for loop fusion

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

This patch *does not* perform loop fusion. Nor do we want it to. We want to deduplicate OpenMP runtime calls that are involved in OpenMP worksharing loops. Please make this clear in the commit message and description. The latter should go into more detail what is happening here and why.

There are various problems wrt. the coding standards. I will only comment on a few but the patch needs to be updated according to the coding standard and the surrounding code patterns.

Why are there no tests?

---

The code is complicated and hard to read, especially given the lack of comments and documentation. Please read through my inline comments. Afterwards, I suggest you start by

1. identifying call sites as the surrounding code does it, then
2. filter call sites that are not legible on their own, then
3. for each call site, find a matching one, if found, replace and rewrite and try again.

You should have test cases that are developed as part of the patch. You should also put early versions for review so you can get feedback.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:127
     Changed |= deleteParallelRegions();
+    Changed |= runTheOMPLoopFusion();
 
----------------
As mentioned in the main message, this does not perform loop fusion. It is a more complex deduplication of runtime calls that may enable loop fusion but for the sake of this patch and this code it is just runtime call deduplication.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:168
+    return false;
+  }
+
----------------
While I am pretty sure the entire map construction is overly complicated and should be replaced, you can achieve the above with something like:

```
for (const auto &It : m)
  if (llvm::any_of(It.second, [I](CallInst *CI) { return I == CI;}))
    return true;
return false;
```


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:221
+    return false;
+  }
+
----------------
Why does it always return false?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:227
+  void runOverTheBlock(Function *F, OMPLoopFusion *OLF) {
+    for (Function::iterator FI = F->begin(); FI != F->end(); ++FI) {
+      for (BasicBlock::iterator BBI = FI->begin(); BBI != FI->end(); ++BBI) {
----------------
Please use modern C++. LLVM is currently based on C++14, for years we allow range based loops:
`for (Instruction *I : instructions(F)) {`


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:230
+        if (CallInst *c = dyn_cast<CallInst>(BBI)) {
+          if (c->getCalledFunction()->getName() == "__kmpc_for_static_init_4") {
+            OLF->current_call_init_instruction = c;
----------------
Please look take a look at `deduplicateRuntimeCalls` and `deleteParallelRegions`. Their we utilize a caching system to find calls to OpenMP runtime functions. It is important that we do not cause a constant overhead in this pass by scanning instructions multiple times.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:239
+          OLF->store_op0_op1.insert(
+              {store->getOperand(1), store->getOperand(0)});
+        }
----------------
Since you are interested in special stores only it would make sense to look for them explicitly, e.g., by following the uses of arguments passed into the `__kmpc_for_static_init_4` call.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:245
+
+  bool cleanInstructions(OMPLoopFusion *OLF) {
+    bool changed = false;
----------------
Most of these functions lack documentation and all of them lack comments explaining the code.
The names need to be adjusted wrt. the coding standard *and* to make them expressive. For example, "clean" is nothing I can associate with an action.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:248
+    errs() << "[starting cleaning]"
+           << "\n";
+    for (auto itr = OLF->call_map.begin(); itr != OLF->call_map.end(); itr++) {
----------------
We cannot have stray debug output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75384