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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits