[PATCH] D126626: [OpenMP][IRBuilder] Add final clause to task

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:627
   /// \param Tied True if the task is tied, false if the task is untied.
   InsertPointTy createTask(const LocationDescription &Loc,
                            InsertPointTy AllocaIP, BodyGenCallbackTy BodyGenCB,
----------------
Please document the new parameter in the doxygen comment.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1259
                             InsertPointTy AllocaIP, BodyGenCallbackTy BodyGenCB,
-                            bool Tied) {
+                            bool Tied, Value *finalBool) {
   if (!updateToLocation(Loc))
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | LLVM naming conventions have parameters start with an upper case letter ]].

Also I don't see the point of the `Bool` suffix. Consider `IsFinal` or `Final`.




================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1340
+    if (finalBool) {
+      Value *finalFlag = Builder.CreateSelect(finalBool, Builder.getInt32(2),
+                                              Builder.getInt32(0));
----------------
[style]


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4590
+  Builder.SetInsertPoint(BodyBB);
+  Value *finalBool = Builder.CreateICmp(
+      CmpInst::Predicate::ICMP_EQ, F->getArg(0),
----------------
[style]


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4611
+  // to zero.
+  EXPECT_TRUE(any_of(OrInst->operands(), [](Value *op) {
+    if (ConstantInt *TiedValue = dyn_cast<ConstantInt>(op))
----------------
[👍] Great use of `any_of`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126626