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

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

I have a few minor questions.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2055
+/// Attach loop metadata \p Properties to the loop described by \p Loop. If the
+/// loop already has metadata, the loop properties are appended.
+static void addLoopMetadata(CanonicalLoopInfo *Loop,
----------------
Nit: In the body, it seems we are prepending.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2168-2169
+  // actually unrolls the loop.
+  UP.Threshold *= 1.5;
+  UP.PartialThreshold *= 1.5;
+
----------------
Should this be settable for experimentation or additional control? If not can you provide an explanation for these values?


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1666
 
+TEST_F(OpenMPIRBuilderTest, UnrollLoopFull) {
+  OpenMPIRBuilder OMPBuilder(*M);
----------------
Are we not calling ompbuilder finalize or verify because it is only adding metadata?


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1735
+
 TEST_F(OpenMPIRBuilderTest, StaticWorkShareLoop) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
----------------
Should we add tests for workshare loops with unroll?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107764