[PATCH] D71989: [OpenMP][IRBuilder] `omp task` support

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1366
+  Builder.SetInsertPoint(UI->getParent());
+  UI->eraseFromParent();
+
----------------
shraiysh wrote:
> Meinersbur wrote:
> > Please avoid temporary instructions. Use `splitBB` instead.
> AFAIK, `splitBB` requires an instruction pointer. I have updated this to erase the temporary instruction immediately after I have split the basic blocks, but I cannot figure out a way to completely eliminate temporary instructions. Is this okay? 
```
BasicBlock *TaskExitBB = splitBB(Builder, "task.exit");
BasicBlock *TaskBodyBB = splitBB(Builder, "task.body");
BasicBlock *TaskAllocaBB = splitBB(Builder, "task.alloca");
```
Note that the reverse order. After this, `Builder` will insert instructions before the terminator of `Currbb` (where you probably don't want to insert instructions here).


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1352
+      WrapperArgTys.push_back(OutlinedFn.getArg(0)->getType());
+    llvm::Twine WrapperFuncName = OutlinedFn.getName() + ".wrapper";
+    SmallString<128> WrapperFuncNameStorage;
----------------
Don't store a Twine in a variable. See comment from `Twine.h`:
```
  /// A Twine is not intended for use directly and should not be stored, its
  /// implementation relies on the ability to store pointers to temporary stack
  /// objects which may be deallocated at the end of a statement. Twines should
  /// only be used accepted as const references in arguments, when an API wishes
  /// to accept possibly-concatenated strings.
```


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1355
+    FunctionCallee WrapperFuncVal = M.getOrInsertFunction(
+        WrapperFuncName.toStringRef(WrapperFuncNameStorage),
+        FunctionType::get(Builder.getInt32Ty(), WrapperArgTys, false));
----------------
No `SmallString<128>` needed.  `str()` creates a `std::string` that implicitly converts to a `llvm::StringRef` that is valid until the end of the statement(`"`;`"`).

Compared to using `std::string` only, this saves the creation of one temporary `std::sting` (for `OutlinedFn.getName()`). Your version saves another one (if fewer than 128 chars), but it is also more complicated. Before `StringRef` was made more compatible to `std::string_view`, the `.str()` wasn't even need.



================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4501
+      dyn_cast<Function>(TaskAllocCall->getArgOperand(5)->stripPointerCasts());
+  EXPECT_NE(WrapperFunc, nullptr);
+  EXPECT_FALSE(WrapperFunc->isDeclaration());
----------------
If `WrapperFunc` is NULL, the next line will be a segfault. `ASSERT_NE` will stop execution if it fails.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4504
+  CallInst *OutlinedFnCall = dyn_cast<CallInst>(WrapperFunc->begin()->begin());
+  EXPECT_NE(OutlinedFnCall, nullptr);
+  EXPECT_EQ(WrapperFunc->getArg(0)->getType(), Builder.getInt32Ty());
----------------
See above (and other occurances checking for nullptr)


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4511-4514
+  EXPECT_TRUE(any_of(instructions(OutlinedFn),
+                     [](Instruction &inst) { return isa<TruncInst>(&inst); }));
+  EXPECT_TRUE(any_of(instructions(OutlinedFn),
+                     [](Instruction &inst) { return isa<ICmpInst>(&inst); }));
----------------
Nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71989