[PATCH] D107764: [OpenMP][OpenMPIRBuilder] Implement loop unrolling.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LG, minor comments below.



================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2639-2643
+    if (UnrolledCLI) {
+      OMPLoopNestStack.push_back(UnrolledCLI);
+    } else {
+      assert(!NeedsUnrolledCLI);
+    }
----------------
make it an unreachable or sink the assert and combine the conditions. Later allows to remove all braces.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2077
+  Latch->getTerminator()->setMetadata(LLVMContext::MD_loop, LoopID);
+}
+
----------------
Nit: Add an assert for latch to exist. Just for my sanity ;)


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2119
+      OptLevel));
+}
+
----------------
Should we cache it in the OpenMPIRBuilder?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2241
+  unsigned Factor = UP.Count;
+  LLVM_DEBUG(dbgs() << "Suggesting unroll factor of " << LoopSize << "\n");
+
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2275
+    return nullptr;
+  }
+
----------------
This code matches the pattern we have in the 2 members above. Can we make it a member as well and make it clear in the name of all 3 that we use metadata and the unroller pass? No strong feelings, more of an idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107764