[Openmp-commits] [PATCH] D71989: [OpenMP][IRBuilder] `omp task` support

[Michael Kruse via Phabricator via Openmp-commits]

Meinersbur added a comment.

FYI, I am revising how outlining works, still waiting on D115216 <https://reviews.llvm.org/D115216> getting reviewed.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1120
+    // for runtime call with a wrapper function.
+    CallInst *StaleCI = cast<CallInst>(OutlinedFn.user_back());
+    bool HasTaskData = StaleCI->arg_size() > 0;
----------------
Could you add an assert checking that there is just a single user, so we do not accidentally a wrong one.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1198
+    Builder.CreateRet(Builder.getInt32(0));
+  };
+
----------------
kiranchandramohan wrote:
> Nit: A debug dump of the IR here might be useful.
Please don't dump complete IR. Too much output makes `-debug` useless.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1197-1198
   EXPECT_TRUE(Arg2Type->isPointerTy());
-  EXPECT_TRUE(cast<PointerType>(Arg2Type)
-                  ->isOpaqueOrPointeeTypeMatches(ArgStructTy));
+  EXPECT_TRUE(
+      cast<PointerType>(Arg2Type)->isOpaqueOrPointeeTypeMatches(ArgStructTy));
 }
----------------
[nit] only whitespace change


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4477
+
+  auto *AllocaBB = Builder.GetInsertBlock();
+  UnreachableInst *UI = Builder.CreateUnreachable();
----------------
[style] LLVM codsing style does not use [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Almost-Always-Auto ]].


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4541-4546
+  for (Instruction &In : instructions(OutlinedFn)) {
+    if (isa<ICmpInst>(&In))
+      FoundIcmp = true;
+    if (isa<TruncInst>(&In))
+      FoundTrunc = true;
+  }
----------------
Consider `llvm::any_of`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71989