[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd23c61490c28: [OpenMP] Introduce the OpenMP-IR-Builder 
(authored by jdoerfert).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Frontend/OpenMP/CMakeLists.txt
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/CMakeLists.txt
  llvm/unittests/Frontend/CMakeLists.txt
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,178 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy =
+FunctionType::get(Type::getVoidTy(Ctx), {Type::getInt32Ty(Ctx)},
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+
+DIBuilder DIB(*M);
+auto File = DIB.createFile("test.dbg", "/");
+auto CU =
+DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+auto SP = DIB.createFunction(
+CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+F->setSubprogram(SP);
+auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+DIB.finalize();
+DL = DebugLoc::get(3, 7, Scope);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+  DebugLoc DL;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotFreeMemory());
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotFreeMemory());
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
+  new UnreachableInst(Ctx, CBB);
+  OMPBuilder.setCancellationBlock(CBB);
+
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  auto NewIP = OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  Builder.restoreIP(NewIP);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 3U);
+  EXPECT_EQ(BB->size(), 4U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:74
+///
+///{
+namespace types {

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Extra comments?
> > I don't know what you want to tell me.
> I mean, you have this `///{` here. Do you need it?
Yes, to help the doxygen documentation.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:174-177
+if (auto *IdentI = dyn_cast(Ident))
+  Call->moveAfter(IdentI);
+else
+  Call->moveBefore(&*Fn->getEntryBlock().getFirstInsertionPt());

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Are you sure it will work correctly with late outlining?
> > Yes.
> Ah, the code fro threadid changed already, probably missed it.
I removed the TODOs now, including the caching.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:74
+///
+///{
+namespace types {

jdoerfert wrote:
> ABataev wrote:
> > Extra comments?
> I don't know what you want to tell me.
I mean, you have this `///{` here. Do you need it?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:174-177
+if (auto *IdentI = dyn_cast(Ident))
+  Call->moveAfter(IdentI);
+else
+  Call->moveBefore(&*Fn->getEntryBlock().getFirstInsertionPt());

jdoerfert wrote:
> ABataev wrote:
> > Are you sure it will work correctly with late outlining?
> Yes.
Ah, the code fro threadid changed already, probably missed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 233429.
jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

Remove call caching in anticipation of D69930 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Frontend/OpenMP/CMakeLists.txt
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/CMakeLists.txt
  llvm/unittests/Frontend/CMakeLists.txt
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,178 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy =
+FunctionType::get(Type::getVoidTy(Ctx), {Type::getInt32Ty(Ctx)},
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+
+DIBuilder DIB(*M);
+auto File = DIB.createFile("test.dbg", "/");
+auto CU =
+DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+auto SP = DIB.createFunction(
+CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+F->setSubprogram(SP);
+auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+DIB.finalize();
+DL = DebugLoc::get(3, 7, Scope);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+  DebugLoc DL;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotFreeMemory());
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotFreeMemory());
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
+  new UnreachableInst(Ctx, CBB);
+  OMPBuilder.setCancellationBlock(CBB);
+
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  auto NewIP = OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  Builder.restoreIP(NewIP);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 3U);
+  EXPECT_EQ(BB->size(), 4U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 6 inline comments as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:74
+///
+///{
+namespace types {

ABataev wrote:
> Extra comments?
I don't know what you want to tell me.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:174-177
+if (auto *IdentI = dyn_cast(Ident))
+  Call->moveAfter(IdentI);
+else
+  Call->moveBefore(&*Fn->getEntryBlock().getFirstInsertionPt());

ABataev wrote:
> Are you sure it will work correctly with late outlining?
Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 60698 tests passed, 1 failed 
and 726 were skipped.

  failed: 
LLVM-Unit.Frontend/_/LLVMFrontendTests/OpenMPIRBuilderTest.CreateCancelBarrier

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 test-results.xml 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:74
+///
+///{
+namespace types {

Extra comments?



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:93
+} // namespace types
+///}
+

Here too.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:174-177
+if (auto *IdentI = dyn_cast(Ident))
+  Call->moveAfter(IdentI);
+else
+  Call->moveBefore(&*Fn->getEntryBlock().getFirstInsertionPt());

Are you sure it will work correctly with late outlining?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:186-188
+  // TODO: Do we really expect these create calls to happen at an invalid
+  //   location and if so is ignoring them the right thing to do? This
+  //   mimics Clang's behavior for now.

Clang drops insert point after some code, like call of `exit()` etc. That's why 
we need to check it in the frontend, otherwise, the compiler crashes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 233418.
jdoerfert added a comment.

Adjust path


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Frontend/OpenMP/CMakeLists.txt
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/CMakeLists.txt
  llvm/unittests/Frontend/CMakeLists.txt
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,178 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy =
+FunctionType::get(Type::getVoidTy(Ctx), {Type::getInt32Ty(Ctx)},
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+
+DIBuilder DIB(*M);
+auto File = DIB.createFile("test.dbg", "/");
+auto CU =
+DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+auto SP = DIB.createFunction(
+CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+F->setSubprogram(SP);
+auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+DIB.finalize();
+DL = DebugLoc::get(3, 7, Scope);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+  DebugLoc DL;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotFreeMemory());
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotFreeMemory());
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
+  new UnreachableInst(Ctx, CBB);
+  OMPBuilder.setCancellationBlock(CBB);
+
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  auto NewIP = OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  Builder.restoreIP(NewIP);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 3U);
+  EXPECT_EQ(BB->size(), 4U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 233409.
jdoerfert added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Frontend/OpenMP/CMakeLists.txt
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/CMakeLists.txt
  llvm/unittests/Frontend/CMakeLists.txt
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,178 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy =
+FunctionType::get(Type::getVoidTy(Ctx), {Type::getInt32Ty(Ctx)},
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+
+DIBuilder DIB(*M);
+auto File = DIB.createFile("test.dbg", "/");
+auto CU =
+DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+auto SP = DIB.createFunction(
+CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+F->setSubprogram(SP);
+auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+DIB.finalize();
+DL = DebugLoc::get(3, 7, Scope);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+  DebugLoc DL;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotFreeMemory());
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotFreeMemory());
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
+  new UnreachableInst(Ctx, CBB);
+  OMPBuilder.setCancellationBlock(CBB);
+
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  auto NewIP = OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  Builder.restoreIP(NewIP);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 3U);
+  EXPECT_EQ(BB->size(), 4U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

In D69785#1763317 , @jdoerfert wrote:

> I'm confused. Was this a review? I'm waiting for a decision here so we can 
> move on and improve on this instead of me modifying it inp-lace two comments 
> at a time.


Explicitly marked as accepted. Patch has looked good for a while and even has 
other people building on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMPKinds.def:165
+
+__OMP_RTL(__kmpc_barrier, false, Void, IdentPtr, Int32)
+__OMP_RTL(__kmpc_cancel_barrier, false, Int32, IdentPtr, Int32)

jdoerfert wrote:
> rogfer01 wrote:
> > As we migrate, we will end with a significant number of interfaces here.
> > 
> > @jdoerfert what do you think about adding a comment with their C prototype 
> > before each one like we do in `clang/lib/CodeGen/CGOpenMPRuntime.cpp`?
> > 
> > Something like this
> > 
> > ```lang=cpp
> > // void __kmpc_barrier(ident_t *loc, kmp_int32 global_tid);
> > __OMP_RTL(__kmpc_barrier, false, Void, IdentPtr, Int32)
> > // kmp_int32 __kmpc_cancel_barrier(ident_t *loc, kmp_int32
> > // global_tid)
> > __OMP_RTL(__kmpc_cancel_barrier, false, Int32, IdentPtr, Int32)
> > ...
> > ```
> I'm fine with this but I doubt it'll help much (compared to the lines we have 
> that show name and types).
> 
> If you want this to happen you should create a patch do add comments for the 
> ones we have here, and others can way in. If there is agreement to apply it, 
> we will do so and continue that tradition from then on. Does that sound good?
Sounds reasonable to me. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.

In D69785#1762438 , @JonChesterfield 
wrote:

> I'd very much like this to land soon. It's the prereq for a lot of other 
> patches and the code looks good.
>
> It's tricky to test the infra before the users are landed so the unit test is 
> particularly appreciated.


I'm confused. Was this a review? I'm waiting for a decision here so we can move 
on and improve on this instead of me modifying it inp-lace two comments at a 
time.




Comment at: llvm/include/llvm/Frontend/OpenMPKinds.def:165
+
+__OMP_RTL(__kmpc_barrier, false, Void, IdentPtr, Int32)
+__OMP_RTL(__kmpc_cancel_barrier, false, Int32, IdentPtr, Int32)

rogfer01 wrote:
> As we migrate, we will end with a significant number of interfaces here.
> 
> @jdoerfert what do you think about adding a comment with their C prototype 
> before each one like we do in `clang/lib/CodeGen/CGOpenMPRuntime.cpp`?
> 
> Something like this
> 
> ```lang=cpp
> // void __kmpc_barrier(ident_t *loc, kmp_int32 global_tid);
> __OMP_RTL(__kmpc_barrier, false, Void, IdentPtr, Int32)
> // kmp_int32 __kmpc_cancel_barrier(ident_t *loc, kmp_int32
> // global_tid)
> __OMP_RTL(__kmpc_cancel_barrier, false, Int32, IdentPtr, Int32)
> ...
> ```
I'm fine with this but I doubt it'll help much (compared to the lines we have 
that show name and types).

If you want this to happen you should create a patch do add comments for the 
ones we have here, and others can way in. If there is agreement to apply it, we 
will do so and continue that tradition from then on. Does that sound good?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMPKinds.def:165
+
+__OMP_RTL(__kmpc_barrier, false, Void, IdentPtr, Int32)
+__OMP_RTL(__kmpc_cancel_barrier, false, Int32, IdentPtr, Int32)

As we migrate, we will end with a significant number of interfaces here.

@jdoerfert what do you think about adding a comment with their C prototype 
before each one like we do in `clang/lib/CodeGen/CGOpenMPRuntime.cpp`?

Something like this

```lang=cpp
// void __kmpc_barrier(ident_t *loc, kmp_int32 global_tid);
__OMP_RTL(__kmpc_barrier, false, Void, IdentPtr, Int32)
// kmp_int32 __kmpc_cancel_barrier(ident_t *loc, kmp_int32
// global_tid)
__OMP_RTL(__kmpc_cancel_barrier, false, Int32, IdentPtr, Int32)
...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I'd very much like this to land soon. It's the prereq for a lot of other 
patches and the code looks good.

It's tricky to test the infra before the users are landed so the unit test is 
particularly appreciated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 230928.
jdoerfert marked 4 inline comments as done.
jdoerfert added a comment.

use streams, remove caching


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/Frontend/OpenMPConstants.h
  llvm/include/llvm/Frontend/OpenMPIRBuilder.h
  llvm/include/llvm/Frontend/OpenMPKinds.def
  llvm/lib/Frontend/CMakeLists.txt
  llvm/lib/Frontend/OpenMPConstants.cpp
  llvm/lib/Frontend/OpenMPIRBuilder.cpp
  llvm/unittests/CMakeLists.txt
  llvm/unittests/Frontend/CMakeLists.txt
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,178 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Frontend/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Frontend/OpenMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy =
+FunctionType::get(Type::getVoidTy(Ctx), {Type::getInt32Ty(Ctx)},
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+
+DIBuilder DIB(*M);
+auto File = DIB.createFile("test.dbg", "/");
+auto CU =
+DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+auto SP = DIB.createFunction(
+CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+F->setSubprogram(SP);
+auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+DIB.finalize();
+DL = DebugLoc::get(3, 7, Scope);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+  DebugLoc DL;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotFreeMemory());
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotFreeMemory());
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
+  new UnreachableInst(Ctx, CBB);
+  OMPBuilder.setCancellationBlock(CBB);
+
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  auto NewIP = OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  Builder.restoreIP(NewIP);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 3U);
+  EXPECT_EQ(BB->size(), 4U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

@ABataev  @JonChesterfield  anything else blocking this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

jdoerfert wrote:
> JonChesterfield wrote:
> > Meinersbur wrote:
> > > JonChesterfield wrote:
> > > > jdoerfert wrote:
> > > > > Meinersbur wrote:
> > > > > > jdoerfert wrote:
> > > > > > > JonChesterfield wrote:
> > > > > > > > Sharing constants between the compiler and the runtime is an 
> > > > > > > > interesting subproblem. I think the cleanest solution is the 
> > > > > > > > one used by libc, where the compiler generates header files 
> > > > > > > > containing the constants which the runtime library includes.
> > > > > > > I'd prefer not to tackle this right now but get this done first 
> > > > > > > and revisit the issue later. OK?
> > > > > > I don't think this is a good solution. It means that libomp cannot 
> > > > > > built built anymore without also building clang. Moreover, the 
> > > > > > values cannot be changed anyway since it would break the ABI.
> > > > > > 
> > > > > > I'd go the other route: The libomp defines what it's ABI is, the 
> > > > > > compiler generates code for it. 
> > > > > This patch doesn't change what we do, just where. The numbers are 
> > > > > hard coded in clang now. Let's start a discussion on the list and if 
> > > > > we come up with a different scheme we do it after this landed.
> > > > Revisit later sounds good.
> > > > 
> > > > @Meinersbur Do you know of an example of a non-llvm compiler using this 
> > > > libomp?
> > > > 
> > > > The usual order is build a compiler, then use it to build the runtime 
> > > > libraries, then the whole package can build other stuff. Provided the 
> > > > compiler doesn't need any of the runtime libraries (compiler-rt, maths 
> > > > libraries, libomp etc) itself the system bootstraps cleanly. Especially 
> > > > important when cross compiling and I suspect the gpu targets in openmp 
> > > > have similarly strict requirements on the first compiler.
> > > > 
> > > > Closely related to that, I tend to assume that the runtime libraries 
> > > > can be rewritten to best serve their only client - the associated 
> > > > compiler - so if libomp is used by out of tree compilers I'd like to 
> > > > know who we are at risk of breaking.
> > > > Do you know of an example of a non-llvm compiler using this libomp?
> > > 
> > > [[ 
> > > https://github.com/llvm-project/llvm-project/blob/master/openmp/runtime/src/kmp_gsupport.cpp
> > >  | gcc  ]] (using libomp's gomp compatibility layer), [[ 
> > > https://www.openmprtl.org/ | icc  ]] (as libomp was initially donated by 
> > > Intel).
> > > 
> > > I don't understand why it even matters if there are other compilers using 
> > > libomp. Every LLVM runtime library can be built stand-alone. 
> > > With constant values being determined during compiler bootstrapping, 
> > > programs built on one computer would be potentially ABI-incompatible with 
> > > a runtime library on another. Think about updating your 
> > > compiler-rt/libomp/libc++ on you computer causing all existing binaries 
> > > on the system to crash because constants changed in the updated 
> > > compiler's bootstrapping process.
> > > 
> > > The only use case I know that does this is are operating system's syscall 
> > > tables. Linux's reference is [[ 
> > > https://github.com/torvalds/linux/blob/master/arch/sh/include/uapi/asm/unistd_64.h
> > >  | unistd.h ]] which is platform-specific and Windows generates the table 
> > > during its [[ https://j00ru.vexillium.org/syscalls/nt/64/ | build process 
> > > ]]. Therefore on Windows, system calls can only be done through ntdll. 
> > > Even on Linux one should use the system's libc instead of directly 
> > > invoking a system call.
> > Thanks. GCC and ICC would presumably be happier with the magic numbers 
> > stored with openmp then (though with the move to a monorepo that's a little 
> > less persuasive).
> > 
> > When constants that affect the ABI change, the result won't work with 
> > existing software regardless of whether the compiler or the library 
> > contains the change. Either the new compiler builds things that don't work 
> > with the old library, or the new library doesn't work with things built by 
> > the old compiler. The two have to agree on the ABI.
> > 
> > At present, openmp does the moral equivalent of #include OpenMPKinds.def 
> > from clang. Moving the constants to libomp means clang will do the 
> > equivalent of #include OpenMPKinds.def from openmp. Breaking that 
> > dependency means making a new subproject that just holds/generates the 
> > constants, that both depend on, which seems more hassle than it's worth. 
> > 
> > I'd like to generate this header as part of the clang build (though 
> > ultimately don't care that much if it's generated as part of the openmp 
> > build) because it's going to become increasingly challenging to read as 
> > non-nvptx architectures are introduced. Likewise it 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 4 inline comments as done.
jdoerfert added a comment.

Anything else?




Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:152-155
+  std::string LineStr = std::to_string(DIL->getLine());
+  std::string ColumnStr = std::to_string(DIL->getColumn());
+  std::string SrcLocStr = (";" + Filename + ";" + Function).str() + ";" +
+  LineStr + ";" + ColumnStr + ";;";

ABataev wrote:
> better to use streams
Sure.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:169-174
+for (Instruction  : Fn->getEntryBlock())
+  if (CallInst *CI = dyn_cast())
+if (CI->getCalledFunction() &&
+CI->getCalledFunction()->getName() == "__kmpc_global_thread_num")
+  return TID = CI;
+

ABataev wrote:
> Will it work with the late outlined parallel regions?
I'll remove the caching here. No need for it given that we will deduplicate 
anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:152-155
+  std::string LineStr = std::to_string(DIL->getLine());
+  std::string ColumnStr = std::to_string(DIL->getColumn());
+  std::string SrcLocStr = (";" + Filename + ";" + Function).str() + ";" +
+  LineStr + ";" + ColumnStr + ";;";

better to use streams



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:169-174
+for (Instruction  : Fn->getEntryBlock())
+  if (CallInst *CI = dyn_cast())
+if (CI->getCalledFunction() &&
+CI->getCalledFunction()->getName() == "__kmpc_global_thread_num")
+  return TID = CI;
+

Will it work with the late outlined parallel regions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > ABataev wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > > > > > Maybe add an assert when the cancellation 
> > > > > > > > > > > > > > > > > > version is requested but the cancellation 
> > > > > > > > > > > > > > > > > > block is not set? Instead of the generating 
> > > > > > > > > > > > > > > > > > simple version of barrier.
> > > > > > > > > > > > > > > > > The interface doesn't work that way as we do 
> > > > > > > > > > > > > > > > > not know here if the cancellation was 
> > > > > > > > > > > > > > > > > requested except if the block was set. That 
> > > > > > > > > > > > > > > > > is basically the flag (and I expect it to 
> > > > > > > > > > > > > > > > > continue to be that way).
> > > > > > > > > > > > > > > > Maybe instead of `ForceSimpleBarrier` add a 
> > > > > > > > > > > > > > > > flag `EmitCancelBarrier` and if it set to true, 
> > > > > > > > > > > > > > > > always emit cancel barrier, otherwise always 
> > > > > > > > > > > > > > > > emit simple barrier? And add an assertion for 
> > > > > > > > > > > > > > > > non-set cancellation block or even accept it as 
> > > > > > > > > > > > > > > > a parameter here.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > Also, what if we have inner exception handling 
> > > > > > > > > > > > > > > > in the region? Will you handle the cleanup 
> > > > > > > > > > > > > > > > correctly in case of the cancelation barrier?
> > > > > > > > > > > > > > > > Maybe instead of ForceSimpleBarrier add a flag 
> > > > > > > > > > > > > > > > EmitCancelBarrier and if it set to true, always 
> > > > > > > > > > > > > > > > emit cancel barrier, otherwise always emit 
> > > > > > > > > > > > > > > > simple barrier? And add an assertion for 
> > > > > > > > > > > > > > > > non-set cancellation block or even accept it as 
> > > > > > > > > > > > > > > > a parameter here.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > What is the difference in moving some of the 
> > > > > > > > > > > > > > > boolean logic to the caller? Also, we have test 
> > > > > > > > > > > > > > > to verify we get cancellation barriers if we need 
> > > > > > > > > > > > > > > them, both unit tests and clang lit tests.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > Also, what if we have inner exception handling 
> > > > > > > > > > > > > > > > in the region? Will you handle the cleanup 
> > > > > > > > > > > > > > > > correctly in case of the cancelation barrier?
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > I think so. Right now through the code in clang 
> > > > > > > > > > > > > > > that does the set up of the cancellation block, 
> > > > > > > > > > > > > > > later through callbacks but we only need that for 
> > > > > > > > > > > > > > > regions where we actually go out of some scope, 
> > > > > > > > > > > > > > > e.g., parallel.
> > > > > > > > > > > > > > 1. I'm just thinking about future users of thus 
> > > > > > > > > > > > > > interface. It woild be good if we could provide 
> > > > > > > > > > > > > > safe interface for all the users, not only clang.
> > > > > > > > > > > > > > 2. Exit out of the OpenMP region is not allowed. 
> > > > > > > > > > > > > > But you may have inner try...catch or just simple 
> > > > > > > > > > > > > > compound statement with local vars that require 
> > > > > > > > > > > > > > constructors/destructors. And the cancellation 
> > > > > > > > > > > > > > barrier may exit out of these regions. And you need 
> > > > > > > > > > > > > > to call all required destructors. You'd better to 
> > > > > > > > > > > > > > think about it now, not later.
> > > > > > > > > > > > > > 2. [...] You'd better to think about it now, not 
> > > > > > > > > > > > > > later.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > First, I do think about it now and I hope this was 
> > > > > > > > > > > > > not an insinuation to suggest otherwise.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > 1. I'm just thinking about future users of thus 
> > > > > > > > > > > > > > interface. It woild be good if 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:200
+OpenMPIRBuilder::InsertPointTy
+OpenMPIRBuilder::emitBarrierImpl(const LocationDescription , Directive 
Kind,
+ bool ForceSimpleCall, bool CheckCancelFlag) {

ABataev wrote:
> Maybe split emission ща `__kmpc_barrier` and `__kmpc_cancel_barrier` 
> functions into 2 independent functions fo the frontends? Rather than rely on 
> the boolean flags?
The frontend doesn't know necessarily when a barrier is issued if it is a 
cancellable, arguably it shouldn't need to know at all

I copied the flags from clang but I will look into removing them eventually 
(and to add a TODO in the meantime).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:200
+OpenMPIRBuilder::InsertPointTy
+OpenMPIRBuilder::emitBarrierImpl(const LocationDescription , Directive 
Kind,
+ bool ForceSimpleCall, bool CheckCancelFlag) {

Maybe split emission ща `__kmpc_barrier` and `__kmpc_cancel_barrier` functions 
into 2 independent functions fo the frontends? Rather than rely on the boolean 
flags?



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > ABataev wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > > > > Maybe add an assert when the cancellation 
> > > > > > > > > > > > > > > > > version is requested but the cancellation 
> > > > > > > > > > > > > > > > > block is not set? Instead of the generating 
> > > > > > > > > > > > > > > > > simple version of barrier.
> > > > > > > > > > > > > > > > The interface doesn't work that way as we do 
> > > > > > > > > > > > > > > > not know here if the cancellation was requested 
> > > > > > > > > > > > > > > > except if the block was set. That is basically 
> > > > > > > > > > > > > > > > the flag (and I expect it to continue to be 
> > > > > > > > > > > > > > > > that way).
> > > > > > > > > > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > > > > > > > > > > `EmitCancelBarrier` and if it set to true, always 
> > > > > > > > > > > > > > > emit cancel barrier, otherwise always emit simple 
> > > > > > > > > > > > > > > barrier? And add an assertion for non-set 
> > > > > > > > > > > > > > > cancellation block or even accept it as a 
> > > > > > > > > > > > > > > parameter here.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Also, what if we have inner exception handling in 
> > > > > > > > > > > > > > > the region? Will you handle the cleanup correctly 
> > > > > > > > > > > > > > > in case of the cancelation barrier?
> > > > > > > > > > > > > > > Maybe instead of ForceSimpleBarrier add a flag 
> > > > > > > > > > > > > > > EmitCancelBarrier and if it set to true, always 
> > > > > > > > > > > > > > > emit cancel barrier, otherwise always emit simple 
> > > > > > > > > > > > > > > barrier? And add an assertion for non-set 
> > > > > > > > > > > > > > > cancellation block or even accept it as a 
> > > > > > > > > > > > > > > parameter here.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > What is the difference in moving some of the 
> > > > > > > > > > > > > > boolean logic to the caller? Also, we have test to 
> > > > > > > > > > > > > > verify we get cancellation barriers if we need 
> > > > > > > > > > > > > > them, both unit tests and clang lit tests.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Also, what if we have inner exception handling in 
> > > > > > > > > > > > > > > the region? Will you handle the cleanup correctly 
> > > > > > > > > > > > > > > in case of the cancelation barrier?
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I think so. Right now through the code in clang 
> > > > > > > > > > > > > > that does the set up of the cancellation block, 
> > > > > > > > > > > > > > later through callbacks but we only need that for 
> > > > > > > > > > > > > > regions where we actually go out of some scope, 
> > > > > > > > > > > > > > e.g., parallel.
> > > > > > > > > > > > > 1. I'm just thinking about future users of thus 
> > > > > > > > > > > > > interface. It woild be good if we could provide safe 
> > > > > > > > > > > > > interface for all the users, not only clang.
> > > > > > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But 
> > > > > > > > > > > > > you may have inner try...catch or just simple 
> > > > > > > > > > > > > compound statement with local vars that require 
> > > > > > > > > > > > > constructors/destructors. And the cancellation 
> > > > > > > > > > > > > barrier may exit out of these regions. And you need 
> > > > > > > > > > > > > to call all required destructors. You'd better to 
> > > > > > > > > > > > > think about it now, not later.
> > > > > > > > > > > > > 2. [...] You'd better to think about it now, not 
> > > > > > > > > > > > > later.
> > > > > > > > > > > > 
> > > > > > > > > > > > First, I do think about it now and I 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > ABataev wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > > > Maybe add an assert when the cancellation 
> > > > > > > > > > > > > > > > version is requested but the cancellation block 
> > > > > > > > > > > > > > > > is not set? Instead of the generating simple 
> > > > > > > > > > > > > > > > version of barrier.
> > > > > > > > > > > > > > > The interface doesn't work that way as we do not 
> > > > > > > > > > > > > > > know here if the cancellation was requested 
> > > > > > > > > > > > > > > except if the block was set. That is basically 
> > > > > > > > > > > > > > > the flag (and I expect it to continue to be that 
> > > > > > > > > > > > > > > way).
> > > > > > > > > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > > > > > > > > > `EmitCancelBarrier` and if it set to true, always 
> > > > > > > > > > > > > > emit cancel barrier, otherwise always emit simple 
> > > > > > > > > > > > > > barrier? And add an assertion for non-set 
> > > > > > > > > > > > > > cancellation block or even accept it as a parameter 
> > > > > > > > > > > > > > here.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Also, what if we have inner exception handling in 
> > > > > > > > > > > > > > the region? Will you handle the cleanup correctly 
> > > > > > > > > > > > > > in case of the cancelation barrier?
> > > > > > > > > > > > > > Maybe instead of ForceSimpleBarrier add a flag 
> > > > > > > > > > > > > > EmitCancelBarrier and if it set to true, always 
> > > > > > > > > > > > > > emit cancel barrier, otherwise always emit simple 
> > > > > > > > > > > > > > barrier? And add an assertion for non-set 
> > > > > > > > > > > > > > cancellation block or even accept it as a parameter 
> > > > > > > > > > > > > > here.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > What is the difference in moving some of the boolean 
> > > > > > > > > > > > > logic to the caller? Also, we have test to verify we 
> > > > > > > > > > > > > get cancellation barriers if we need them, both unit 
> > > > > > > > > > > > > tests and clang lit tests.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > Also, what if we have inner exception handling in 
> > > > > > > > > > > > > > the region? Will you handle the cleanup correctly 
> > > > > > > > > > > > > > in case of the cancelation barrier?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I think so. Right now through the code in clang that 
> > > > > > > > > > > > > does the set up of the cancellation block, later 
> > > > > > > > > > > > > through callbacks but we only need that for regions 
> > > > > > > > > > > > > where we actually go out of some scope, e.g., 
> > > > > > > > > > > > > parallel.
> > > > > > > > > > > > 1. I'm just thinking about future users of thus 
> > > > > > > > > > > > interface. It woild be good if we could provide safe 
> > > > > > > > > > > > interface for all the users, not only clang.
> > > > > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But 
> > > > > > > > > > > > you may have inner try...catch or just simple compound 
> > > > > > > > > > > > statement with local vars that require 
> > > > > > > > > > > > constructors/destructors. And the cancellation barrier 
> > > > > > > > > > > > may exit out of these regions. And you need to call all 
> > > > > > > > > > > > required destructors. You'd better to think about it 
> > > > > > > > > > > > now, not later.
> > > > > > > > > > > > 2. [...] You'd better to think about it now, not later.
> > > > > > > > > > > 
> > > > > > > > > > > First, I do think about it now and I hope this was not an 
> > > > > > > > > > > insinuation to suggest otherwise.
> > > > > > > > > > > 
> > > > > > > > > > > > 1. I'm just thinking about future users of thus 
> > > > > > > > > > > > interface. It woild be good if we could provide safe 
> > > > > > > > > > > > interface for all the users, not only clang.
> > > > > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But 
> > > > > > > > > > > > you may have inner try...catch or just simple compound 
> > > > > > > > > > > > statement with local vars that require 
> > > > > > > > > > > > constructors/destructors. And the cancellation barrier 
> > > > 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > > Maybe add an assert when the cancellation version 
> > > > > > > > > > > > > > > is requested but the cancellation block is not 
> > > > > > > > > > > > > > > set? Instead of the generating simple version of 
> > > > > > > > > > > > > > > barrier.
> > > > > > > > > > > > > > The interface doesn't work that way as we do not 
> > > > > > > > > > > > > > know here if the cancellation was requested except 
> > > > > > > > > > > > > > if the block was set. That is basically the flag 
> > > > > > > > > > > > > > (and I expect it to continue to be that way).
> > > > > > > > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > > > > > > > > `EmitCancelBarrier` and if it set to true, always 
> > > > > > > > > > > > > emit cancel barrier, otherwise always emit simple 
> > > > > > > > > > > > > barrier? And add an assertion for non-set 
> > > > > > > > > > > > > cancellation block or even accept it as a parameter 
> > > > > > > > > > > > > here.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Also, what if we have inner exception handling in the 
> > > > > > > > > > > > > region? Will you handle the cleanup correctly in case 
> > > > > > > > > > > > > of the cancelation barrier?
> > > > > > > > > > > > > Maybe instead of ForceSimpleBarrier add a flag 
> > > > > > > > > > > > > EmitCancelBarrier and if it set to true, always emit 
> > > > > > > > > > > > > cancel barrier, otherwise always emit simple barrier? 
> > > > > > > > > > > > > And add an assertion for non-set cancellation block 
> > > > > > > > > > > > > or even accept it as a parameter here.
> > > > > > > > > > > > 
> > > > > > > > > > > > What is the difference in moving some of the boolean 
> > > > > > > > > > > > logic to the caller? Also, we have test to verify we 
> > > > > > > > > > > > get cancellation barriers if we need them, both unit 
> > > > > > > > > > > > tests and clang lit tests.
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > > Also, what if we have inner exception handling in the 
> > > > > > > > > > > > > region? Will you handle the cleanup correctly in case 
> > > > > > > > > > > > > of the cancelation barrier?
> > > > > > > > > > > > 
> > > > > > > > > > > > I think so. Right now through the code in clang that 
> > > > > > > > > > > > does the set up of the cancellation block, later 
> > > > > > > > > > > > through callbacks but we only need that for regions 
> > > > > > > > > > > > where we actually go out of some scope, e.g., parallel.
> > > > > > > > > > > 1. I'm just thinking about future users of thus 
> > > > > > > > > > > interface. It woild be good if we could provide safe 
> > > > > > > > > > > interface for all the users, not only clang.
> > > > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you 
> > > > > > > > > > > may have inner try...catch or just simple compound 
> > > > > > > > > > > statement with local vars that require 
> > > > > > > > > > > constructors/destructors. And the cancellation barrier 
> > > > > > > > > > > may exit out of these regions. And you need to call all 
> > > > > > > > > > > required destructors. You'd better to think about it now, 
> > > > > > > > > > > not later.
> > > > > > > > > > > 2. [...] You'd better to think about it now, not later.
> > > > > > > > > > 
> > > > > > > > > > First, I do think about it now and I hope this was not an 
> > > > > > > > > > insinuation to suggest otherwise.
> > > > > > > > > > 
> > > > > > > > > > > 1. I'm just thinking about future users of thus 
> > > > > > > > > > > interface. It woild be good if we could provide safe 
> > > > > > > > > > > interface for all the users, not only clang.
> > > > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you 
> > > > > > > > > > > may have inner try...catch or just simple compound 
> > > > > > > > > > > statement with local vars that require 
> > > > > > > > > > > constructors/destructors. And the cancellation barrier 
> > > > > > > > > > > may exit out of these regions. And you need to call all 
> > > > > > > > > > > required destructors.
> > > > > > > > > > 
> > > > > > > > > > Generally speaking, we shall not add features that 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

jdoerfert wrote:
> ABataev wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > Maybe add an assert when the cancellation version 
> > > > > > > > > > > > > > is requested but the cancellation block is not set? 
> > > > > > > > > > > > > > Instead of the generating simple version of barrier.
> > > > > > > > > > > > > The interface doesn't work that way as we do not know 
> > > > > > > > > > > > > here if the cancellation was requested except if the 
> > > > > > > > > > > > > block was set. That is basically the flag (and I 
> > > > > > > > > > > > > expect it to continue to be that way).
> > > > > > > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > > > > > > > `EmitCancelBarrier` and if it set to true, always emit 
> > > > > > > > > > > > cancel barrier, otherwise always emit simple barrier? 
> > > > > > > > > > > > And add an assertion for non-set cancellation block or 
> > > > > > > > > > > > even accept it as a parameter here.
> > > > > > > > > > > > 
> > > > > > > > > > > > Also, what if we have inner exception handling in the 
> > > > > > > > > > > > region? Will you handle the cleanup correctly in case 
> > > > > > > > > > > > of the cancelation barrier?
> > > > > > > > > > > > Maybe instead of ForceSimpleBarrier add a flag 
> > > > > > > > > > > > EmitCancelBarrier and if it set to true, always emit 
> > > > > > > > > > > > cancel barrier, otherwise always emit simple barrier? 
> > > > > > > > > > > > And add an assertion for non-set cancellation block or 
> > > > > > > > > > > > even accept it as a parameter here.
> > > > > > > > > > > 
> > > > > > > > > > > What is the difference in moving some of the boolean 
> > > > > > > > > > > logic to the caller? Also, we have test to verify we get 
> > > > > > > > > > > cancellation barriers if we need them, both unit tests 
> > > > > > > > > > > and clang lit tests.
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > > Also, what if we have inner exception handling in the 
> > > > > > > > > > > > region? Will you handle the cleanup correctly in case 
> > > > > > > > > > > > of the cancelation barrier?
> > > > > > > > > > > 
> > > > > > > > > > > I think so. Right now through the code in clang that does 
> > > > > > > > > > > the set up of the cancellation block, later through 
> > > > > > > > > > > callbacks but we only need that for regions where we 
> > > > > > > > > > > actually go out of some scope, e.g., parallel.
> > > > > > > > > > 1. I'm just thinking about future users of thus interface. 
> > > > > > > > > > It woild be good if we could provide safe interface for all 
> > > > > > > > > > the users, not only clang.
> > > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you 
> > > > > > > > > > may have inner try...catch or just simple compound 
> > > > > > > > > > statement with local vars that require 
> > > > > > > > > > constructors/destructors. And the cancellation barrier may 
> > > > > > > > > > exit out of these regions. And you need to call all 
> > > > > > > > > > required destructors. You'd better to think about it now, 
> > > > > > > > > > not later.
> > > > > > > > > > 2. [...] You'd better to think about it now, not later.
> > > > > > > > > 
> > > > > > > > > First, I do think about it now and I hope this was not an 
> > > > > > > > > insinuation to suggest otherwise.
> > > > > > > > > 
> > > > > > > > > > 1. I'm just thinking about future users of thus interface. 
> > > > > > > > > > It woild be good if we could provide safe interface for all 
> > > > > > > > > > the users, not only clang.
> > > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you 
> > > > > > > > > > may have inner try...catch or just simple compound 
> > > > > > > > > > statement with local vars that require 
> > > > > > > > > > constructors/destructors. And the cancellation barrier may 
> > > > > > > > > > exit out of these regions. And you need to call all 
> > > > > > > > > > required destructors.
> > > > > > > > > 
> > > > > > > > > Generally speaking, we shall not add features that we cannot 
> > > > > > > > > use or test with the assumption we will use them in the 
> > > > > > > > > future. This is suggested by the LLVM best practices. If you 
> > > > > > > > > have specific changes in mind that are testable and better 
> > > > > > > > > than what 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> ABataev wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > Maybe add an assert when the cancellation version is 
> > > > > > > > > > > > > requested but the cancellation block is not set? 
> > > > > > > > > > > > > Instead of the generating simple version of barrier.
> > > > > > > > > > > > The interface doesn't work that way as we do not know 
> > > > > > > > > > > > here if the cancellation was requested except if the 
> > > > > > > > > > > > block was set. That is basically the flag (and I expect 
> > > > > > > > > > > > it to continue to be that way).
> > > > > > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > > > > > > `EmitCancelBarrier` and if it set to true, always emit 
> > > > > > > > > > > cancel barrier, otherwise always emit simple barrier? And 
> > > > > > > > > > > add an assertion for non-set cancellation block or even 
> > > > > > > > > > > accept it as a parameter here.
> > > > > > > > > > > 
> > > > > > > > > > > Also, what if we have inner exception handling in the 
> > > > > > > > > > > region? Will you handle the cleanup correctly in case of 
> > > > > > > > > > > the cancelation barrier?
> > > > > > > > > > > Maybe instead of ForceSimpleBarrier add a flag 
> > > > > > > > > > > EmitCancelBarrier and if it set to true, always emit 
> > > > > > > > > > > cancel barrier, otherwise always emit simple barrier? And 
> > > > > > > > > > > add an assertion for non-set cancellation block or even 
> > > > > > > > > > > accept it as a parameter here.
> > > > > > > > > > 
> > > > > > > > > > What is the difference in moving some of the boolean logic 
> > > > > > > > > > to the caller? Also, we have test to verify we get 
> > > > > > > > > > cancellation barriers if we need them, both unit tests and 
> > > > > > > > > > clang lit tests.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > Also, what if we have inner exception handling in the 
> > > > > > > > > > > region? Will you handle the cleanup correctly in case of 
> > > > > > > > > > > the cancelation barrier?
> > > > > > > > > > 
> > > > > > > > > > I think so. Right now through the code in clang that does 
> > > > > > > > > > the set up of the cancellation block, later through 
> > > > > > > > > > callbacks but we only need that for regions where we 
> > > > > > > > > > actually go out of some scope, e.g., parallel.
> > > > > > > > > 1. I'm just thinking about future users of thus interface. It 
> > > > > > > > > woild be good if we could provide safe interface for all the 
> > > > > > > > > users, not only clang.
> > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you may 
> > > > > > > > > have inner try...catch or just simple compound statement with 
> > > > > > > > > local vars that require constructors/destructors. And the 
> > > > > > > > > cancellation barrier may exit out of these regions. And you 
> > > > > > > > > need to call all required destructors. You'd better to think 
> > > > > > > > > about it now, not later.
> > > > > > > > > 2. [...] You'd better to think about it now, not later.
> > > > > > > > 
> > > > > > > > First, I do think about it now and I hope this was not an 
> > > > > > > > insinuation to suggest otherwise.
> > > > > > > > 
> > > > > > > > > 1. I'm just thinking about future users of thus interface. It 
> > > > > > > > > woild be good if we could provide safe interface for all the 
> > > > > > > > > users, not only clang.
> > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you may 
> > > > > > > > > have inner try...catch or just simple compound statement with 
> > > > > > > > > local vars that require constructors/destructors. And the 
> > > > > > > > > cancellation barrier may exit out of these regions. And you 
> > > > > > > > > need to call all required destructors.
> > > > > > > > 
> > > > > > > > Generally speaking, we shall not add features that we cannot 
> > > > > > > > use or test with the assumption we will use them in the future. 
> > > > > > > > This is suggested by the LLVM best practices. If you have 
> > > > > > > > specific changes in mind that are testable and better than what 
> > > > > > > > I suggested so far, please bring them forward. You can also 
> > > > > > > > bring forward suggestions on how it might look in the future 
> > > > > > > > but without 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > Maybe add an assert when the cancellation version is 
> > > > > > > > > > > > requested but the cancellation block is not set? 
> > > > > > > > > > > > Instead of the generating simple version of barrier.
> > > > > > > > > > > The interface doesn't work that way as we do not know 
> > > > > > > > > > > here if the cancellation was requested except if the 
> > > > > > > > > > > block was set. That is basically the flag (and I expect 
> > > > > > > > > > > it to continue to be that way).
> > > > > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > > > > > `EmitCancelBarrier` and if it set to true, always emit 
> > > > > > > > > > cancel barrier, otherwise always emit simple barrier? And 
> > > > > > > > > > add an assertion for non-set cancellation block or even 
> > > > > > > > > > accept it as a parameter here.
> > > > > > > > > > 
> > > > > > > > > > Also, what if we have inner exception handling in the 
> > > > > > > > > > region? Will you handle the cleanup correctly in case of 
> > > > > > > > > > the cancelation barrier?
> > > > > > > > > > Maybe instead of ForceSimpleBarrier add a flag 
> > > > > > > > > > EmitCancelBarrier and if it set to true, always emit cancel 
> > > > > > > > > > barrier, otherwise always emit simple barrier? And add an 
> > > > > > > > > > assertion for non-set cancellation block or even accept it 
> > > > > > > > > > as a parameter here.
> > > > > > > > > 
> > > > > > > > > What is the difference in moving some of the boolean logic to 
> > > > > > > > > the caller? Also, we have test to verify we get cancellation 
> > > > > > > > > barriers if we need them, both unit tests and clang lit tests.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > Also, what if we have inner exception handling in the 
> > > > > > > > > > region? Will you handle the cleanup correctly in case of 
> > > > > > > > > > the cancelation barrier?
> > > > > > > > > 
> > > > > > > > > I think so. Right now through the code in clang that does the 
> > > > > > > > > set up of the cancellation block, later through callbacks but 
> > > > > > > > > we only need that for regions where we actually go out of 
> > > > > > > > > some scope, e.g., parallel.
> > > > > > > > 1. I'm just thinking about future users of thus interface. It 
> > > > > > > > woild be good if we could provide safe interface for all the 
> > > > > > > > users, not only clang.
> > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you may 
> > > > > > > > have inner try...catch or just simple compound statement with 
> > > > > > > > local vars that require constructors/destructors. And the 
> > > > > > > > cancellation barrier may exit out of these regions. And you 
> > > > > > > > need to call all required destructors. You'd better to think 
> > > > > > > > about it now, not later.
> > > > > > > > 2. [...] You'd better to think about it now, not later.
> > > > > > > 
> > > > > > > First, I do think about it now and I hope this was not an 
> > > > > > > insinuation to suggest otherwise.
> > > > > > > 
> > > > > > > > 1. I'm just thinking about future users of thus interface. It 
> > > > > > > > woild be good if we could provide safe interface for all the 
> > > > > > > > users, not only clang.
> > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you may 
> > > > > > > > have inner try...catch or just simple compound statement with 
> > > > > > > > local vars that require constructors/destructors. And the 
> > > > > > > > cancellation barrier may exit out of these regions. And you 
> > > > > > > > need to call all required destructors.
> > > > > > > 
> > > > > > > Generally speaking, we shall not add features that we cannot use 
> > > > > > > or test with the assumption we will use them in the future. This 
> > > > > > > is suggested by the LLVM best practices. If you have specific 
> > > > > > > changes in mind that are testable and better than what I 
> > > > > > > suggested so far, please bring them forward. You can also bring 
> > > > > > > forward suggestions on how it might look in the future but 
> > > > > > > without a real use case now it is not practical to block a review 
> > > > > > > based on that, given that we can change the interface once the 
> > > > > > > time has come.
> > > > > > > 
> > > > > > > I said before, we will need callbacks for 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > Maybe add an assert when the cancellation version is 
> > > > > > > > > > requested but the cancellation block is not set? Instead of 
> > > > > > > > > > the generating simple version of barrier.
> > > > > > > > > The interface doesn't work that way as we do not know here if 
> > > > > > > > > the cancellation was requested except if the block was set. 
> > > > > > > > > That is basically the flag (and I expect it to continue to be 
> > > > > > > > > that way).
> > > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > > > `EmitCancelBarrier` and if it set to true, always emit cancel 
> > > > > > > > barrier, otherwise always emit simple barrier? And add an 
> > > > > > > > assertion for non-set cancellation block or even accept it as a 
> > > > > > > > parameter here.
> > > > > > > > 
> > > > > > > > Also, what if we have inner exception handling in the region? 
> > > > > > > > Will you handle the cleanup correctly in case of the 
> > > > > > > > cancelation barrier?
> > > > > > > > Maybe instead of ForceSimpleBarrier add a flag 
> > > > > > > > EmitCancelBarrier and if it set to true, always emit cancel 
> > > > > > > > barrier, otherwise always emit simple barrier? And add an 
> > > > > > > > assertion for non-set cancellation block or even accept it as a 
> > > > > > > > parameter here.
> > > > > > > 
> > > > > > > What is the difference in moving some of the boolean logic to the 
> > > > > > > caller? Also, we have test to verify we get cancellation barriers 
> > > > > > > if we need them, both unit tests and clang lit tests.
> > > > > > > 
> > > > > > > 
> > > > > > > > Also, what if we have inner exception handling in the region? 
> > > > > > > > Will you handle the cleanup correctly in case of the 
> > > > > > > > cancelation barrier?
> > > > > > > 
> > > > > > > I think so. Right now through the code in clang that does the set 
> > > > > > > up of the cancellation block, later through callbacks but we only 
> > > > > > > need that for regions where we actually go out of some scope, 
> > > > > > > e.g., parallel.
> > > > > > 1. I'm just thinking about future users of thus interface. It woild 
> > > > > > be good if we could provide safe interface for all the users, not 
> > > > > > only clang.
> > > > > > 2. Exit out of the OpenMP region is not allowed. But you may have 
> > > > > > inner try...catch or just simple compound statement with local vars 
> > > > > > that require constructors/destructors. And the cancellation barrier 
> > > > > > may exit out of these regions. And you need to call all required 
> > > > > > destructors. You'd better to think about it now, not later.
> > > > > > 2. [...] You'd better to think about it now, not later.
> > > > > 
> > > > > First, I do think about it now and I hope this was not an insinuation 
> > > > > to suggest otherwise.
> > > > > 
> > > > > > 1. I'm just thinking about future users of thus interface. It woild 
> > > > > > be good if we could provide safe interface for all the users, not 
> > > > > > only clang.
> > > > > > 2. Exit out of the OpenMP region is not allowed. But you may have 
> > > > > > inner try...catch or just simple compound statement with local vars 
> > > > > > that require constructors/destructors. And the cancellation barrier 
> > > > > > may exit out of these regions. And you need to call all required 
> > > > > > destructors.
> > > > > 
> > > > > Generally speaking, we shall not add features that we cannot use or 
> > > > > test with the assumption we will use them in the future. This is 
> > > > > suggested by the LLVM best practices. If you have specific changes in 
> > > > > mind that are testable and better than what I suggested so far, 
> > > > > please bring them forward. You can also bring forward suggestions on 
> > > > > how it might look in the future but without a real use case now it is 
> > > > > not practical to block a review based on that, given that we can 
> > > > > change the interface once the time has come.
> > > > > 
> > > > > I said before, we will need callbacks for destructors, actual 
> > > > > handling of cancellation blocks, and there are various other features 
> > > > > missing right now. Nevertheless, we cannot build them into the 
> > > > > current interface, or even try to prepare for all of them, while 
> > > > > keeping the patches small and concise.
> > > > It won't work for clang, I'm afraid. You need a list of 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > Maybe add an assert when the cancellation version is 
> > > > > > > > > > > requested but the cancellation block is not set? Instead 
> > > > > > > > > > > of the generating simple version of barrier.
> > > > > > > > > > The interface doesn't work that way as we do not know here 
> > > > > > > > > > if the cancellation was requested except if the block was 
> > > > > > > > > > set. That is basically the flag (and I expect it to 
> > > > > > > > > > continue to be that way).
> > > > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > > > > `EmitCancelBarrier` and if it set to true, always emit cancel 
> > > > > > > > > barrier, otherwise always emit simple barrier? And add an 
> > > > > > > > > assertion for non-set cancellation block or even accept it as 
> > > > > > > > > a parameter here.
> > > > > > > > > 
> > > > > > > > > Also, what if we have inner exception handling in the region? 
> > > > > > > > > Will you handle the cleanup correctly in case of the 
> > > > > > > > > cancelation barrier?
> > > > > > > > > Maybe instead of ForceSimpleBarrier add a flag 
> > > > > > > > > EmitCancelBarrier and if it set to true, always emit cancel 
> > > > > > > > > barrier, otherwise always emit simple barrier? And add an 
> > > > > > > > > assertion for non-set cancellation block or even accept it as 
> > > > > > > > > a parameter here.
> > > > > > > > 
> > > > > > > > What is the difference in moving some of the boolean logic to 
> > > > > > > > the caller? Also, we have test to verify we get cancellation 
> > > > > > > > barriers if we need them, both unit tests and clang lit tests.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > Also, what if we have inner exception handling in the region? 
> > > > > > > > > Will you handle the cleanup correctly in case of the 
> > > > > > > > > cancelation barrier?
> > > > > > > > 
> > > > > > > > I think so. Right now through the code in clang that does the 
> > > > > > > > set up of the cancellation block, later through callbacks but 
> > > > > > > > we only need that for regions where we actually go out of some 
> > > > > > > > scope, e.g., parallel.
> > > > > > > 1. I'm just thinking about future users of thus interface. It 
> > > > > > > woild be good if we could provide safe interface for all the 
> > > > > > > users, not only clang.
> > > > > > > 2. Exit out of the OpenMP region is not allowed. But you may have 
> > > > > > > inner try...catch or just simple compound statement with local 
> > > > > > > vars that require constructors/destructors. And the cancellation 
> > > > > > > barrier may exit out of these regions. And you need to call all 
> > > > > > > required destructors. You'd better to think about it now, not 
> > > > > > > later.
> > > > > > > 2. [...] You'd better to think about it now, not later.
> > > > > > 
> > > > > > First, I do think about it now and I hope this was not an 
> > > > > > insinuation to suggest otherwise.
> > > > > > 
> > > > > > > 1. I'm just thinking about future users of thus interface. It 
> > > > > > > woild be good if we could provide safe interface for all the 
> > > > > > > users, not only clang.
> > > > > > > 2. Exit out of the OpenMP region is not allowed. But you may have 
> > > > > > > inner try...catch or just simple compound statement with local 
> > > > > > > vars that require constructors/destructors. And the cancellation 
> > > > > > > barrier may exit out of these regions. And you need to call all 
> > > > > > > required destructors.
> > > > > > 
> > > > > > Generally speaking, we shall not add features that we cannot use or 
> > > > > > test with the assumption we will use them in the future. This is 
> > > > > > suggested by the LLVM best practices. If you have specific changes 
> > > > > > in mind that are testable and better than what I suggested so far, 
> > > > > > please bring them forward. You can also bring forward suggestions 
> > > > > > on how it might look in the future but without a real use case now 
> > > > > > it is not practical to block a review based on that, given that we 
> > > > > > can change the interface once the time has come.
> > > > > > 
> > > > > > I said before, we will need callbacks for destructors, actual 
> > > > > > handling of cancellation blocks, and there are various other 
> > > > > > features missing right now. Nevertheless, we cannot build them into 
> > > > > > 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > Maybe add an assert when the cancellation version is 
> > > > > > > > > requested but the cancellation block is not set? Instead of 
> > > > > > > > > the generating simple version of barrier.
> > > > > > > > The interface doesn't work that way as we do not know here if 
> > > > > > > > the cancellation was requested except if the block was set. 
> > > > > > > > That is basically the flag (and I expect it to continue to be 
> > > > > > > > that way).
> > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > > `EmitCancelBarrier` and if it set to true, always emit cancel 
> > > > > > > barrier, otherwise always emit simple barrier? And add an 
> > > > > > > assertion for non-set cancellation block or even accept it as a 
> > > > > > > parameter here.
> > > > > > > 
> > > > > > > Also, what if we have inner exception handling in the region? 
> > > > > > > Will you handle the cleanup correctly in case of the cancelation 
> > > > > > > barrier?
> > > > > > > Maybe instead of ForceSimpleBarrier add a flag EmitCancelBarrier 
> > > > > > > and if it set to true, always emit cancel barrier, otherwise 
> > > > > > > always emit simple barrier? And add an assertion for non-set 
> > > > > > > cancellation block or even accept it as a parameter here.
> > > > > > 
> > > > > > What is the difference in moving some of the boolean logic to the 
> > > > > > caller? Also, we have test to verify we get cancellation barriers 
> > > > > > if we need them, both unit tests and clang lit tests.
> > > > > > 
> > > > > > 
> > > > > > > Also, what if we have inner exception handling in the region? 
> > > > > > > Will you handle the cleanup correctly in case of the cancelation 
> > > > > > > barrier?
> > > > > > 
> > > > > > I think so. Right now through the code in clang that does the set 
> > > > > > up of the cancellation block, later through callbacks but we only 
> > > > > > need that for regions where we actually go out of some scope, e.g., 
> > > > > > parallel.
> > > > > 1. I'm just thinking about future users of thus interface. It woild 
> > > > > be good if we could provide safe interface for all the users, not 
> > > > > only clang.
> > > > > 2. Exit out of the OpenMP region is not allowed. But you may have 
> > > > > inner try...catch or just simple compound statement with local vars 
> > > > > that require constructors/destructors. And the cancellation barrier 
> > > > > may exit out of these regions. And you need to call all required 
> > > > > destructors. You'd better to think about it now, not later.
> > > > > 2. [...] You'd better to think about it now, not later.
> > > > 
> > > > First, I do think about it now and I hope this was not an insinuation 
> > > > to suggest otherwise.
> > > > 
> > > > > 1. I'm just thinking about future users of thus interface. It woild 
> > > > > be good if we could provide safe interface for all the users, not 
> > > > > only clang.
> > > > > 2. Exit out of the OpenMP region is not allowed. But you may have 
> > > > > inner try...catch or just simple compound statement with local vars 
> > > > > that require constructors/destructors. And the cancellation barrier 
> > > > > may exit out of these regions. And you need to call all required 
> > > > > destructors.
> > > > 
> > > > Generally speaking, we shall not add features that we cannot use or 
> > > > test with the assumption we will use them in the future. This is 
> > > > suggested by the LLVM best practices. If you have specific changes in 
> > > > mind that are testable and better than what I suggested so far, please 
> > > > bring them forward. You can also bring forward suggestions on how it 
> > > > might look in the future but without a real use case now it is not 
> > > > practical to block a review based on that, given that we can change the 
> > > > interface once the time has come.
> > > > 
> > > > I said before, we will need callbacks for destructors, actual handling 
> > > > of cancellation blocks, and there are various other features missing 
> > > > right now. Nevertheless, we cannot build them into the current 
> > > > interface, or even try to prepare for all of them, while keeping the 
> > > > patches small and concise.
> > > It won't work for clang, I'm afraid. You need a list of desructors here. 
> > > But clang uses recursive codegen and it is very hard to walk over the 
> > > call tree and gather all required destructors into a 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > Maybe add an assert when the cancellation version is requested 
> > > > > > > > but the cancellation block is not set? Instead of the 
> > > > > > > > generating simple version of barrier.
> > > > > > > The interface doesn't work that way as we do not know here if the 
> > > > > > > cancellation was requested except if the block was set. That is 
> > > > > > > basically the flag (and I expect it to continue to be that way).
> > > > > > Maybe instead of `ForceSimpleBarrier` add a flag 
> > > > > > `EmitCancelBarrier` and if it set to true, always emit cancel 
> > > > > > barrier, otherwise always emit simple barrier? And add an assertion 
> > > > > > for non-set cancellation block or even accept it as a parameter 
> > > > > > here.
> > > > > > 
> > > > > > Also, what if we have inner exception handling in the region? Will 
> > > > > > you handle the cleanup correctly in case of the cancelation barrier?
> > > > > > Maybe instead of ForceSimpleBarrier add a flag EmitCancelBarrier 
> > > > > > and if it set to true, always emit cancel barrier, otherwise always 
> > > > > > emit simple barrier? And add an assertion for non-set cancellation 
> > > > > > block or even accept it as a parameter here.
> > > > > 
> > > > > What is the difference in moving some of the boolean logic to the 
> > > > > caller? Also, we have test to verify we get cancellation barriers if 
> > > > > we need them, both unit tests and clang lit tests.
> > > > > 
> > > > > 
> > > > > > Also, what if we have inner exception handling in the region? Will 
> > > > > > you handle the cleanup correctly in case of the cancelation barrier?
> > > > > 
> > > > > I think so. Right now through the code in clang that does the set up 
> > > > > of the cancellation block, later through callbacks but we only need 
> > > > > that for regions where we actually go out of some scope, e.g., 
> > > > > parallel.
> > > > 1. I'm just thinking about future users of thus interface. It woild be 
> > > > good if we could provide safe interface for all the users, not only 
> > > > clang.
> > > > 2. Exit out of the OpenMP region is not allowed. But you may have inner 
> > > > try...catch or just simple compound statement with local vars that 
> > > > require constructors/destructors. And the cancellation barrier may exit 
> > > > out of these regions. And you need to call all required destructors. 
> > > > You'd better to think about it now, not later.
> > > > 2. [...] You'd better to think about it now, not later.
> > > 
> > > First, I do think about it now and I hope this was not an insinuation to 
> > > suggest otherwise.
> > > 
> > > > 1. I'm just thinking about future users of thus interface. It woild be 
> > > > good if we could provide safe interface for all the users, not only 
> > > > clang.
> > > > 2. Exit out of the OpenMP region is not allowed. But you may have inner 
> > > > try...catch or just simple compound statement with local vars that 
> > > > require constructors/destructors. And the cancellation barrier may exit 
> > > > out of these regions. And you need to call all required destructors.
> > > 
> > > Generally speaking, we shall not add features that we cannot use or test 
> > > with the assumption we will use them in the future. This is suggested by 
> > > the LLVM best practices. If you have specific changes in mind that are 
> > > testable and better than what I suggested so far, please bring them 
> > > forward. You can also bring forward suggestions on how it might look in 
> > > the future but without a real use case now it is not practical to block a 
> > > review based on that, given that we can change the interface once the 
> > > time has come.
> > > 
> > > I said before, we will need callbacks for destructors, actual handling of 
> > > cancellation blocks, and there are various other features missing right 
> > > now. Nevertheless, we cannot build them into the current interface, or 
> > > even try to prepare for all of them, while keeping the patches small and 
> > > concise.
> > It won't work for clang, I'm afraid. You need a list of desructors here. 
> > But clang uses recursive codegen and it is very hard to walk over the call 
> > tree and gather all required destructors into a list. At least, it will 
> > require significant rework in clang frontend.
> > Instead of generating the branch to cancellation block in the builder, I 
> > would suggest to call a single callback function provided by the frontend, 
> > which will generate correct branch 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > Maybe add an assert when the cancellation version is requested 
> > > > > > > but the cancellation block is not set? Instead of the generating 
> > > > > > > simple version of barrier.
> > > > > > The interface doesn't work that way as we do not know here if the 
> > > > > > cancellation was requested except if the block was set. That is 
> > > > > > basically the flag (and I expect it to continue to be that way).
> > > > > Maybe instead of `ForceSimpleBarrier` add a flag `EmitCancelBarrier` 
> > > > > and if it set to true, always emit cancel barrier, otherwise always 
> > > > > emit simple barrier? And add an assertion for non-set cancellation 
> > > > > block or even accept it as a parameter here.
> > > > > 
> > > > > Also, what if we have inner exception handling in the region? Will 
> > > > > you handle the cleanup correctly in case of the cancelation barrier?
> > > > > Maybe instead of ForceSimpleBarrier add a flag EmitCancelBarrier and 
> > > > > if it set to true, always emit cancel barrier, otherwise always emit 
> > > > > simple barrier? And add an assertion for non-set cancellation block 
> > > > > or even accept it as a parameter here.
> > > > 
> > > > What is the difference in moving some of the boolean logic to the 
> > > > caller? Also, we have test to verify we get cancellation barriers if we 
> > > > need them, both unit tests and clang lit tests.
> > > > 
> > > > 
> > > > > Also, what if we have inner exception handling in the region? Will 
> > > > > you handle the cleanup correctly in case of the cancelation barrier?
> > > > 
> > > > I think so. Right now through the code in clang that does the set up of 
> > > > the cancellation block, later through callbacks but we only need that 
> > > > for regions where we actually go out of some scope, e.g., parallel.
> > > 1. I'm just thinking about future users of thus interface. It woild be 
> > > good if we could provide safe interface for all the users, not only clang.
> > > 2. Exit out of the OpenMP region is not allowed. But you may have inner 
> > > try...catch or just simple compound statement with local vars that 
> > > require constructors/destructors. And the cancellation barrier may exit 
> > > out of these regions. And you need to call all required destructors. 
> > > You'd better to think about it now, not later.
> > > 2. [...] You'd better to think about it now, not later.
> > 
> > First, I do think about it now and I hope this was not an insinuation to 
> > suggest otherwise.
> > 
> > > 1. I'm just thinking about future users of thus interface. It woild be 
> > > good if we could provide safe interface for all the users, not only clang.
> > > 2. Exit out of the OpenMP region is not allowed. But you may have inner 
> > > try...catch or just simple compound statement with local vars that 
> > > require constructors/destructors. And the cancellation barrier may exit 
> > > out of these regions. And you need to call all required destructors.
> > 
> > Generally speaking, we shall not add features that we cannot use or test 
> > with the assumption we will use them in the future. This is suggested by 
> > the LLVM best practices. If you have specific changes in mind that are 
> > testable and better than what I suggested so far, please bring them 
> > forward. You can also bring forward suggestions on how it might look in the 
> > future but without a real use case now it is not practical to block a 
> > review based on that, given that we can change the interface once the time 
> > has come.
> > 
> > I said before, we will need callbacks for destructors, actual handling of 
> > cancellation blocks, and there are various other features missing right 
> > now. Nevertheless, we cannot build them into the current interface, or even 
> > try to prepare for all of them, while keeping the patches small and concise.
> It won't work for clang, I'm afraid. You need a list of desructors here. But 
> clang uses recursive codegen and it is very hard to walk over the call tree 
> and gather all required destructors into a list. At least, it will require 
> significant rework in clang frontend.
> Instead of generating the branch to cancellation block in the builder, I 
> would suggest to call a single callback function provided by the frontend, 
> which will generate correct branch over a chain of the destructor blocks. In 
> this case, you won't need this cancellation block at all. This is what I 
> meant when said that you need to think about this 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > Maybe add an assert when the cancellation version is requested but 
> > > > > > the cancellation block is not set? Instead of the generating simple 
> > > > > > version of barrier.
> > > > > The interface doesn't work that way as we do not know here if the 
> > > > > cancellation was requested except if the block was set. That is 
> > > > > basically the flag (and I expect it to continue to be that way).
> > > > Maybe instead of `ForceSimpleBarrier` add a flag `EmitCancelBarrier` 
> > > > and if it set to true, always emit cancel barrier, otherwise always 
> > > > emit simple barrier? And add an assertion for non-set cancellation 
> > > > block or even accept it as a parameter here.
> > > > 
> > > > Also, what if we have inner exception handling in the region? Will you 
> > > > handle the cleanup correctly in case of the cancelation barrier?
> > > > Maybe instead of ForceSimpleBarrier add a flag EmitCancelBarrier and if 
> > > > it set to true, always emit cancel barrier, otherwise always emit 
> > > > simple barrier? And add an assertion for non-set cancellation block or 
> > > > even accept it as a parameter here.
> > > 
> > > What is the difference in moving some of the boolean logic to the caller? 
> > > Also, we have test to verify we get cancellation barriers if we need 
> > > them, both unit tests and clang lit tests.
> > > 
> > > 
> > > > Also, what if we have inner exception handling in the region? Will you 
> > > > handle the cleanup correctly in case of the cancelation barrier?
> > > 
> > > I think so. Right now through the code in clang that does the set up of 
> > > the cancellation block, later through callbacks but we only need that for 
> > > regions where we actually go out of some scope, e.g., parallel.
> > 1. I'm just thinking about future users of thus interface. It woild be good 
> > if we could provide safe interface for all the users, not only clang.
> > 2. Exit out of the OpenMP region is not allowed. But you may have inner 
> > try...catch or just simple compound statement with local vars that require 
> > constructors/destructors. And the cancellation barrier may exit out of 
> > these regions. And you need to call all required destructors. You'd better 
> > to think about it now, not later.
> > 2. [...] You'd better to think about it now, not later.
> 
> First, I do think about it now and I hope this was not an insinuation to 
> suggest otherwise.
> 
> > 1. I'm just thinking about future users of thus interface. It woild be good 
> > if we could provide safe interface for all the users, not only clang.
> > 2. Exit out of the OpenMP region is not allowed. But you may have inner 
> > try...catch or just simple compound statement with local vars that require 
> > constructors/destructors. And the cancellation barrier may exit out of 
> > these regions. And you need to call all required destructors.
> 
> Generally speaking, we shall not add features that we cannot use or test with 
> the assumption we will use them in the future. This is suggested by the LLVM 
> best practices. If you have specific changes in mind that are testable and 
> better than what I suggested so far, please bring them forward. You can also 
> bring forward suggestions on how it might look in the future but without a 
> real use case now it is not practical to block a review based on that, given 
> that we can change the interface once the time has come.
> 
> I said before, we will need callbacks for destructors, actual handling of 
> cancellation blocks, and there are various other features missing right now. 
> Nevertheless, we cannot build them into the current interface, or even try to 
> prepare for all of them, while keeping the patches small and concise.
It won't work for clang, I'm afraid. You need a list of desructors here. But 
clang uses recursive codegen and it is very hard to walk over the call tree and 
gather all required destructors into a list. At least, it will require 
significant rework in clang frontend.
Instead of generating the branch to cancellation block in the builder, I would 
suggest to call a single callback function provided by the frontend, which will 
generate correct branch over a chain of the destructor blocks. In this case, 
you won't need this cancellation block at all. This is what I meant when said 
that you need to think about this problem right now. That current solution is 
not very suitable for the use in the frontend.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > Maybe add an assert when the cancellation version is requested but 
> > > > > the cancellation block is not set? Instead of the generating simple 
> > > > > version of barrier.
> > > > The interface doesn't work that way as we do not know here if the 
> > > > cancellation was requested except if the block was set. That is 
> > > > basically the flag (and I expect it to continue to be that way).
> > > Maybe instead of `ForceSimpleBarrier` add a flag `EmitCancelBarrier` and 
> > > if it set to true, always emit cancel barrier, otherwise always emit 
> > > simple barrier? And add an assertion for non-set cancellation block or 
> > > even accept it as a parameter here.
> > > 
> > > Also, what if we have inner exception handling in the region? Will you 
> > > handle the cleanup correctly in case of the cancelation barrier?
> > > Maybe instead of ForceSimpleBarrier add a flag EmitCancelBarrier and if 
> > > it set to true, always emit cancel barrier, otherwise always emit simple 
> > > barrier? And add an assertion for non-set cancellation block or even 
> > > accept it as a parameter here.
> > 
> > What is the difference in moving some of the boolean logic to the caller? 
> > Also, we have test to verify we get cancellation barriers if we need them, 
> > both unit tests and clang lit tests.
> > 
> > 
> > > Also, what if we have inner exception handling in the region? Will you 
> > > handle the cleanup correctly in case of the cancelation barrier?
> > 
> > I think so. Right now through the code in clang that does the set up of the 
> > cancellation block, later through callbacks but we only need that for 
> > regions where we actually go out of some scope, e.g., parallel.
> 1. I'm just thinking about future users of thus interface. It woild be good 
> if we could provide safe interface for all the users, not only clang.
> 2. Exit out of the OpenMP region is not allowed. But you may have inner 
> try...catch or just simple compound statement with local vars that require 
> constructors/destructors. And the cancellation barrier may exit out of these 
> regions. And you need to call all required destructors. You'd better to think 
> about it now, not later.
> 2. [...] You'd better to think about it now, not later.

First, I do think about it now and I hope this was not an insinuation to 
suggest otherwise.

> 1. I'm just thinking about future users of thus interface. It woild be good 
> if we could provide safe interface for all the users, not only clang.
> 2. Exit out of the OpenMP region is not allowed. But you may have inner 
> try...catch or just simple compound statement with local vars that require 
> constructors/destructors. And the cancellation barrier may exit out of these 
> regions. And you need to call all required destructors.

Generally speaking, we shall not add features that we cannot use or test with 
the assumption we will use them in the future. This is suggested by the LLVM 
best practices. If you have specific changes in mind that are testable and 
better than what I suggested so far, please bring them forward. You can also 
bring forward suggestions on how it might look in the future but without a real 
use case now it is not practical to block a review based on that, given that we 
can change the interface once the time has come.

I said before, we will need callbacks for destructors, actual handling of 
cancellation blocks, and there are various other features missing right now. 
Nevertheless, we cannot build them into the current interface, or even try to 
prepare for all of them, while keeping the patches small and concise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > Maybe add an assert when the cancellation version is requested but the 
> > > > cancellation block is not set? Instead of the generating simple version 
> > > > of barrier.
> > > The interface doesn't work that way as we do not know here if the 
> > > cancellation was requested except if the block was set. That is basically 
> > > the flag (and I expect it to continue to be that way).
> > Maybe instead of `ForceSimpleBarrier` add a flag `EmitCancelBarrier` and if 
> > it set to true, always emit cancel barrier, otherwise always emit simple 
> > barrier? And add an assertion for non-set cancellation block or even accept 
> > it as a parameter here.
> > 
> > Also, what if we have inner exception handling in the region? Will you 
> > handle the cleanup correctly in case of the cancelation barrier?
> > Maybe instead of ForceSimpleBarrier add a flag EmitCancelBarrier and if it 
> > set to true, always emit cancel barrier, otherwise always emit simple 
> > barrier? And add an assertion for non-set cancellation block or even accept 
> > it as a parameter here.
> 
> What is the difference in moving some of the boolean logic to the caller? 
> Also, we have test to verify we get cancellation barriers if we need them, 
> both unit tests and clang lit tests.
> 
> 
> > Also, what if we have inner exception handling in the region? Will you 
> > handle the cleanup correctly in case of the cancelation barrier?
> 
> I think so. Right now through the code in clang that does the set up of the 
> cancellation block, later through callbacks but we only need that for regions 
> where we actually go out of some scope, e.g., parallel.
1. I'm just thinking about future users of thus interface. It woild be good if 
we could provide safe interface for all the users, not only clang.
2. Exit out of the OpenMP region is not allowed. But you may have inner 
try...catch or just simple compound statement with local vars that require 
constructors/destructors. And the cancellation barrier may exit out of these 
regions. And you need to call all required destructors. You'd better to think 
about it now, not later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Maybe add an assert when the cancellation version is requested but the 
> > > cancellation block is not set? Instead of the generating simple version 
> > > of barrier.
> > The interface doesn't work that way as we do not know here if the 
> > cancellation was requested except if the block was set. That is basically 
> > the flag (and I expect it to continue to be that way).
> Maybe instead of `ForceSimpleBarrier` add a flag `EmitCancelBarrier` and if 
> it set to true, always emit cancel barrier, otherwise always emit simple 
> barrier? And add an assertion for non-set cancellation block or even accept 
> it as a parameter here.
> 
> Also, what if we have inner exception handling in the region? Will you handle 
> the cleanup correctly in case of the cancelation barrier?
> Maybe instead of ForceSimpleBarrier add a flag EmitCancelBarrier and if it 
> set to true, always emit cancel barrier, otherwise always emit simple 
> barrier? And add an assertion for non-set cancellation block or even accept 
> it as a parameter here.

What is the difference in moving some of the boolean logic to the caller? Also, 
we have test to verify we get cancellation barriers if we need them, both unit 
tests and clang lit tests.


> Also, what if we have inner exception handling in the region? Will you handle 
> the cleanup correctly in case of the cancelation barrier?

I think so. Right now through the code in clang that does the set up of the 
cancellation block, later through callbacks but we only need that for regions 
where we actually go out of some scope, e.g., parallel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

jdoerfert wrote:
> ABataev wrote:
> > Maybe add an assert when the cancellation version is requested but the 
> > cancellation block is not set? Instead of the generating simple version of 
> > barrier.
> The interface doesn't work that way as we do not know here if the 
> cancellation was requested except if the block was set. That is basically the 
> flag (and I expect it to continue to be that way).
Maybe instead of `ForceSimpleBarrier` add a flag `EmitCancelBarrier` and if it 
set to true, always emit cancel barrier, otherwise always emit simple barrier? 
And add an assertion for non-set cancellation block or even accept it as a 
parameter here.

Also, what if we have inner exception handling in the region? Will you handle 
the cleanup correctly in case of the cancelation barrier?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 3 inline comments as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

JonChesterfield wrote:
> Meinersbur wrote:
> > JonChesterfield wrote:
> > > jdoerfert wrote:
> > > > Meinersbur wrote:
> > > > > jdoerfert wrote:
> > > > > > JonChesterfield wrote:
> > > > > > > Sharing constants between the compiler and the runtime is an 
> > > > > > > interesting subproblem. I think the cleanest solution is the one 
> > > > > > > used by libc, where the compiler generates header files 
> > > > > > > containing the constants which the runtime library includes.
> > > > > > I'd prefer not to tackle this right now but get this done first and 
> > > > > > revisit the issue later. OK?
> > > > > I don't think this is a good solution. It means that libomp cannot 
> > > > > built built anymore without also building clang. Moreover, the values 
> > > > > cannot be changed anyway since it would break the ABI.
> > > > > 
> > > > > I'd go the other route: The libomp defines what it's ABI is, the 
> > > > > compiler generates code for it. 
> > > > This patch doesn't change what we do, just where. The numbers are hard 
> > > > coded in clang now. Let's start a discussion on the list and if we come 
> > > > up with a different scheme we do it after this landed.
> > > Revisit later sounds good.
> > > 
> > > @Meinersbur Do you know of an example of a non-llvm compiler using this 
> > > libomp?
> > > 
> > > The usual order is build a compiler, then use it to build the runtime 
> > > libraries, then the whole package can build other stuff. Provided the 
> > > compiler doesn't need any of the runtime libraries (compiler-rt, maths 
> > > libraries, libomp etc) itself the system bootstraps cleanly. Especially 
> > > important when cross compiling and I suspect the gpu targets in openmp 
> > > have similarly strict requirements on the first compiler.
> > > 
> > > Closely related to that, I tend to assume that the runtime libraries can 
> > > be rewritten to best serve their only client - the associated compiler - 
> > > so if libomp is used by out of tree compilers I'd like to know who we are 
> > > at risk of breaking.
> > > Do you know of an example of a non-llvm compiler using this libomp?
> > 
> > [[ 
> > https://github.com/llvm-project/llvm-project/blob/master/openmp/runtime/src/kmp_gsupport.cpp
> >  | gcc  ]] (using libomp's gomp compatibility layer), [[ 
> > https://www.openmprtl.org/ | icc  ]] (as libomp was initially donated by 
> > Intel).
> > 
> > I don't understand why it even matters if there are other compilers using 
> > libomp. Every LLVM runtime library can be built stand-alone. 
> > With constant values being determined during compiler bootstrapping, 
> > programs built on one computer would be potentially ABI-incompatible with a 
> > runtime library on another. Think about updating your 
> > compiler-rt/libomp/libc++ on you computer causing all existing binaries on 
> > the system to crash because constants changed in the updated compiler's 
> > bootstrapping process.
> > 
> > The only use case I know that does this is are operating system's syscall 
> > tables. Linux's reference is [[ 
> > https://github.com/torvalds/linux/blob/master/arch/sh/include/uapi/asm/unistd_64.h
> >  | unistd.h ]] which is platform-specific and Windows generates the table 
> > during its [[ https://j00ru.vexillium.org/syscalls/nt/64/ | build process 
> > ]]. Therefore on Windows, system calls can only be done through ntdll. Even 
> > on Linux one should use the system's libc instead of directly invoking a 
> > system call.
> Thanks. GCC and ICC would presumably be happier with the magic numbers stored 
> with openmp then (though with the move to a monorepo that's a little less 
> persuasive).
> 
> When constants that affect the ABI change, the result won't work with 
> existing software regardless of whether the compiler or the library contains 
> the change. Either the new compiler builds things that don't work with the 
> old library, or the new library doesn't work with things built by the old 
> compiler. The two have to agree on the ABI.
> 
> At present, openmp does the moral equivalent of #include OpenMPKinds.def from 
> clang. Moving the constants to libomp means clang will do the equivalent of 
> #include OpenMPKinds.def from openmp. Breaking that dependency means making a 
> new subproject that just holds/generates the constants, that both depend on, 
> which seems more hassle than it's worth. 
> 
> I'd like to generate this header as part of the clang build (though 
> ultimately don't care that much if it's generated as part of the openmp 
> build) because it's going to become increasingly challenging to read as 
> non-nvptx architectures are introduced. Likewise it would be useful to 
> generate the interface.h for deviceRTL (or equivalently a set of unit tests 
> checking the function types) from the same 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:179
+Builder.CreateCall(FnDecl, Ident, "omp_global_thread_num");
+if (Instruction *IdentI = dyn_cast(Ident))
+  Call->moveAfter(IdentI);

`auto *`



Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(

Maybe add an assert when the cancellation version is requested but the 
cancellation block is not set? Instead of the generating simple version of 
barrier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Uncertainty over the handling of constant data between clang and libopenmp not 
withstanding, I think this is good to go.




Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

Meinersbur wrote:
> JonChesterfield wrote:
> > jdoerfert wrote:
> > > Meinersbur wrote:
> > > > jdoerfert wrote:
> > > > > JonChesterfield wrote:
> > > > > > Sharing constants between the compiler and the runtime is an 
> > > > > > interesting subproblem. I think the cleanest solution is the one 
> > > > > > used by libc, where the compiler generates header files containing 
> > > > > > the constants which the runtime library includes.
> > > > > I'd prefer not to tackle this right now but get this done first and 
> > > > > revisit the issue later. OK?
> > > > I don't think this is a good solution. It means that libomp cannot 
> > > > built built anymore without also building clang. Moreover, the values 
> > > > cannot be changed anyway since it would break the ABI.
> > > > 
> > > > I'd go the other route: The libomp defines what it's ABI is, the 
> > > > compiler generates code for it. 
> > > This patch doesn't change what we do, just where. The numbers are hard 
> > > coded in clang now. Let's start a discussion on the list and if we come 
> > > up with a different scheme we do it after this landed.
> > Revisit later sounds good.
> > 
> > @Meinersbur Do you know of an example of a non-llvm compiler using this 
> > libomp?
> > 
> > The usual order is build a compiler, then use it to build the runtime 
> > libraries, then the whole package can build other stuff. Provided the 
> > compiler doesn't need any of the runtime libraries (compiler-rt, maths 
> > libraries, libomp etc) itself the system bootstraps cleanly. Especially 
> > important when cross compiling and I suspect the gpu targets in openmp have 
> > similarly strict requirements on the first compiler.
> > 
> > Closely related to that, I tend to assume that the runtime libraries can be 
> > rewritten to best serve their only client - the associated compiler - so if 
> > libomp is used by out of tree compilers I'd like to know who we are at risk 
> > of breaking.
> > Do you know of an example of a non-llvm compiler using this libomp?
> 
> [[ 
> https://github.com/llvm-project/llvm-project/blob/master/openmp/runtime/src/kmp_gsupport.cpp
>  | gcc  ]] (using libomp's gomp compatibility layer), [[ 
> https://www.openmprtl.org/ | icc  ]] (as libomp was initially donated by 
> Intel).
> 
> I don't understand why it even matters if there are other compilers using 
> libomp. Every LLVM runtime library can be built stand-alone. 
> With constant values being determined during compiler bootstrapping, programs 
> built on one computer would be potentially ABI-incompatible with a runtime 
> library on another. Think about updating your compiler-rt/libomp/libc++ on 
> you computer causing all existing binaries on the system to crash because 
> constants changed in the updated compiler's bootstrapping process.
> 
> The only use case I know that does this is are operating system's syscall 
> tables. Linux's reference is [[ 
> https://github.com/torvalds/linux/blob/master/arch/sh/include/uapi/asm/unistd_64.h
>  | unistd.h ]] which is platform-specific and Windows generates the table 
> during its [[ https://j00ru.vexillium.org/syscalls/nt/64/ | build process ]]. 
> Therefore on Windows, system calls can only be done through ntdll. Even on 
> Linux one should use the system's libc instead of directly invoking a system 
> call.
Thanks. GCC and ICC would presumably be happier with the magic numbers stored 
with openmp then (though with the move to a monorepo that's a little less 
persuasive).

When constants that affect the ABI change, the result won't work with existing 
software regardless of whether the compiler or the library contains the change. 
Either the new compiler builds things that don't work with the old library, or 
the new library doesn't work with things built by the old compiler. The two 
have to agree on the ABI.

At present, openmp does the moral equivalent of #include OpenMPKinds.def from 
clang. Moving the constants to libomp means clang will do the equivalent of 
#include OpenMPKinds.def from openmp. Breaking that dependency means making a 
new subproject that just holds/generates the constants, that both depend on, 
which seems more hassle than it's worth. 

I'd like to generate this header as part of the clang build (though ultimately 
don't care that much if it's generated as part of the openmp build) because 
it's going to become increasingly challenging to read as non-nvptx 
architectures are introduced. Likewise it would be useful to generate the 
interface.h for deviceRTL (or equivalently a set of unit tests checking the 
function types) from the same source to ensure it matches and that's not 
economically feasible within the C preprocessor.


Repository:
  rG LLVM 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228792.
jdoerfert added a comment.

Add cancel_barrier functionality + test, move everything to "Frontend"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/Frontend/OpenMPConstants.h
  llvm/include/llvm/Frontend/OpenMPKinds.def
  llvm/include/llvm/Transforms/Utils/OpenMPIRBuilder.h
  llvm/lib/Frontend/CMakeLists.txt
  llvm/lib/Frontend/OpenMPConstants.cpp
  llvm/lib/Frontend/OpenMPIRBuilder.cpp
  llvm/unittests/CMakeLists.txt
  llvm/unittests/Frontend/CMakeLists.txt
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,178 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Transforms/Utils/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Frontend/OpenMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy =
+FunctionType::get(Type::getVoidTy(Ctx), {Type::getInt32Ty(Ctx)},
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+
+DIBuilder DIB(*M);
+auto File = DIB.createFile("test.dbg", "/");
+auto CU =
+DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+auto SP = DIB.createFunction(
+CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+F->setSubprogram(SP);
+auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+DIB.finalize();
+DL = DebugLoc::get(3, 7, Scope);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+  DebugLoc DL;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotFreeMemory());
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotFreeMemory());
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, CreateCancelBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
+  new UnreachableInst(Ctx, CBB);
+  OMPBuilder.setCancellationBlock(CBB);
+
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  auto NewIP = OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  Builder.restoreIP(NewIP);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 3U);
+  EXPECT_EQ(BB->size(), 4U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

JonChesterfield wrote:
> jdoerfert wrote:
> > Meinersbur wrote:
> > > jdoerfert wrote:
> > > > JonChesterfield wrote:
> > > > > Sharing constants between the compiler and the runtime is an 
> > > > > interesting subproblem. I think the cleanest solution is the one used 
> > > > > by libc, where the compiler generates header files containing the 
> > > > > constants which the runtime library includes.
> > > > I'd prefer not to tackle this right now but get this done first and 
> > > > revisit the issue later. OK?
> > > I don't think this is a good solution. It means that libomp cannot built 
> > > built anymore without also building clang. Moreover, the values cannot be 
> > > changed anyway since it would break the ABI.
> > > 
> > > I'd go the other route: The libomp defines what it's ABI is, the compiler 
> > > generates code for it. 
> > This patch doesn't change what we do, just where. The numbers are hard 
> > coded in clang now. Let's start a discussion on the list and if we come up 
> > with a different scheme we do it after this landed.
> Revisit later sounds good.
> 
> @Meinersbur Do you know of an example of a non-llvm compiler using this 
> libomp?
> 
> The usual order is build a compiler, then use it to build the runtime 
> libraries, then the whole package can build other stuff. Provided the 
> compiler doesn't need any of the runtime libraries (compiler-rt, maths 
> libraries, libomp etc) itself the system bootstraps cleanly. Especially 
> important when cross compiling and I suspect the gpu targets in openmp have 
> similarly strict requirements on the first compiler.
> 
> Closely related to that, I tend to assume that the runtime libraries can be 
> rewritten to best serve their only client - the associated compiler - so if 
> libomp is used by out of tree compilers I'd like to know who we are at risk 
> of breaking.
> Do you know of an example of a non-llvm compiler using this libomp?

[[ 
https://github.com/llvm-project/llvm-project/blob/master/openmp/runtime/src/kmp_gsupport.cpp
 | gcc  ]] (using libomp's gomp compatibility layer), [[ 
https://www.openmprtl.org/ | icc  ]] (as libomp was initially donated by Intel).

I don't understand why it even matters if there are other compilers using 
libomp. Every LLVM runtime library can be built stand-alone. 
With constant values being determined during compiler bootstrapping, programs 
built on one computer would be potentially ABI-incompatible with a runtime 
library on another. Think about updating your compiler-rt/libomp/libc++ on you 
computer causing all existing binaries on the system to crash because constants 
changed in the updated compiler's bootstrapping process.

The only use case I know that does this is are operating system's syscall 
tables. Linux's reference is [[ 
https://github.com/torvalds/linux/blob/master/arch/sh/include/uapi/asm/unistd_64.h
 | unistd.h ]] which is platform-specific and Windows generates the table 
during its [[ https://j00ru.vexillium.org/syscalls/nt/64/ | build process ]]. 
Therefore on Windows, system calls can only be done through ntdll. Even on 
Linux one should use the system's libc instead of directly invoking a system 
call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPConstants.h:57
+#include "llvm/IR/OpenMPKinds.def"
+  LLVM_MARK_AS_BITMASK_ENUM(0x7FFF)
+};

ABataev wrote:
> Why do you use `0x7FFF` as the largest value?
Because it is a valid upper bound. We don't have all values in OMP_IDENT_FLAG 
but if you want to come up with a tighter bound, please feel free.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:49-51
+  default:
+// Attributes are optional.
+break;

ABataev wrote:
> Do you need the `default:` here?
If we want attributes to be optional, then yes. If we say they are mandatory, 
then not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPConstants.h:57
+#include "llvm/IR/OpenMPKinds.def"
+  LLVM_MARK_AS_BITMASK_ENUM(0x7FFF)
+};

Why do you use `0x7FFF` as the largest value?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:49-51
+  default:
+// Attributes are optional.
+break;

Do you need the `default:` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228754.
jdoerfert marked an inline comment as done.
jdoerfert added a comment.

Make attributes opt-in


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp
  llvm/lib/IR/OpenMPIRBuilder.cpp
  llvm/unittests/IR/CMakeLists.txt
  llvm/unittests/IR/IRBuilderTest.cpp
  llvm/unittests/IR/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/IR/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/IR/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,133 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy = FunctionType::get(Type::getVoidTy(Ctx),
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotFreeMemory());
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(Barrier->getCalledFunction()->doesNotFreeMemory());
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, DbgLoc) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+
+  IRBuilder<> Builder(BB);
+
+  DIBuilder DIB(*M);
+  auto File = DIB.createFile("test.dbg", "/");
+  auto CU =
+  DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+  auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+  auto SP = DIB.createFunction(
+  CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+  DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+  F->setSubprogram(SP);
+  auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+  DIB.finalize();
+
+  DebugLoc DL = DebugLoc::get(3, 7, Scope);
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  CallInst *GTID = dyn_cast(>front());
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_EQ(GTID->getDebugLoc(), DL);
+  EXPECT_EQ(Barrier->getDebugLoc(), DL);
+  EXPECT_TRUE(isa(Barrier->getOperand(0)));
+  if (!isa(Barrier->getOperand(0)))
+return;
+  GlobalVariable *Ident = cast(Barrier->getOperand(0));
+  EXPECT_TRUE(Ident->hasInitializer());
+  if (!Ident->hasInitializer())
+return;
+  Constant *Initializer = Ident->getInitializer();
+  EXPECT_TRUE(
+  isa(Initializer->getOperand(4)->stripPointerCasts()));
+  GlobalVariable *SrcStrGlob =
+  cast(Initializer->getOperand(4)->stripPointerCasts());
+  if (!SrcStrGlob)
+

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

jdoerfert wrote:
> Meinersbur wrote:
> > jdoerfert wrote:
> > > JonChesterfield wrote:
> > > > Sharing constants between the compiler and the runtime is an 
> > > > interesting subproblem. I think the cleanest solution is the one used 
> > > > by libc, where the compiler generates header files containing the 
> > > > constants which the runtime library includes.
> > > I'd prefer not to tackle this right now but get this done first and 
> > > revisit the issue later. OK?
> > I don't think this is a good solution. It means that libomp cannot built 
> > built anymore without also building clang. Moreover, the values cannot be 
> > changed anyway since it would break the ABI.
> > 
> > I'd go the other route: The libomp defines what it's ABI is, the compiler 
> > generates code for it. 
> This patch doesn't change what we do, just where. The numbers are hard coded 
> in clang now. Let's start a discussion on the list and if we come up with a 
> different scheme we do it after this landed.
Revisit later sounds good.

@Meinersbur Do you know of an example of a non-llvm compiler using this libomp?

The usual order is build a compiler, then use it to build the runtime 
libraries, then the whole package can build other stuff. Provided the compiler 
doesn't need any of the runtime libraries (compiler-rt, maths libraries, libomp 
etc) itself the system bootstraps cleanly. Especially important when cross 
compiling and I suspect the gpu targets in openmp have similarly strict 
requirements on the first compiler.

Closely related to that, I tend to assume that the runtime libraries can be 
rewritten to best serve their only client - the associated compiler - so if 
libomp is used by out of tree compilers I'd like to know who we are at risk of 
breaking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:153-159
+// Search the entry block, not needed once all thread id calls go through
+// here and are cached in the OpenMPIRBuilder.
+for (Instruction  : Fn->getEntryBlock())
+  if (CallInst *CI = dyn_cast())
+if (CI->getCalledFunction() &&
+CI->getCalledFunction()->getName() == "__kmpc_global_thread_num")
+  return TID = CI;

ABataev wrote:
> Do you really need this if you have a deduplication pass?
Not once the pass is in. I'll remove this with the pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:153-159
+// Search the entry block, not needed once all thread id calls go through
+// here and are cached in the OpenMPIRBuilder.
+for (Instruction  : Fn->getEntryBlock())
+  if (CallInst *CI = dyn_cast())
+if (CI->getCalledFunction() &&
+CI->getCalledFunction()->getName() == "__kmpc_global_thread_num")
+  return TID = CI;

Do you really need this if you have a deduplication pass?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228364.
jdoerfert added a comment.

Do not eagerly create function declarations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp
  llvm/lib/IR/OpenMPIRBuilder.cpp
  llvm/unittests/IR/CMakeLists.txt
  llvm/unittests/IR/IRBuilderTest.cpp
  llvm/unittests/IR/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/IR/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/IR/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,129 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy = FunctionType::get(Type::getVoidTy(Ctx),
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 6U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 6U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, DbgLoc) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+
+  IRBuilder<> Builder(BB);
+
+  DIBuilder DIB(*M);
+  auto File = DIB.createFile("test.dbg", "/");
+  auto CU =
+  DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+  auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+  auto SP = DIB.createFunction(
+  CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+  DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+  F->setSubprogram(SP);
+  auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+  DIB.finalize();
+
+  DebugLoc DL = DebugLoc::get(3, 7, Scope);
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  CallInst *GTID = dyn_cast(>front());
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_EQ(GTID->getDebugLoc(), DL);
+  EXPECT_EQ(Barrier->getDebugLoc(), DL);
+  EXPECT_TRUE(isa(Barrier->getOperand(0)));
+  if (!isa(Barrier->getOperand(0)))
+return;
+  GlobalVariable *Ident = cast(Barrier->getOperand(0));
+  EXPECT_TRUE(Ident->hasInitializer());
+  if (!Ident->hasInitializer())
+return;
+  Constant *Initializer = Ident->getInitializer();
+  EXPECT_TRUE(
+  isa(Initializer->getOperand(4)->stripPointerCasts()));
+  GlobalVariable *SrcStrGlob =
+  cast(Initializer->getOperand(4)->stripPointerCasts());
+  if (!SrcStrGlob)
+return;
+  EXPECT_TRUE(isa(SrcStrGlob->getInitializer()));
+  ConstantDataArray *SrcSrc =
+  dyn_cast(SrcStrGlob->getInitializer());
+  if (!SrcSrc)
+return;
+  EXPECT_EQ(SrcSrc->getAsCString(), ";test.dbg;foo;3;7;;");
+}
+} // namespace
Index: llvm/unittests/IR/IRBuilderTest.cpp

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228327.
jdoerfert added a comment.

Adjust RTL attributes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp
  llvm/lib/IR/OpenMPIRBuilder.cpp
  llvm/unittests/IR/CMakeLists.txt
  llvm/unittests/IR/IRBuilderTest.cpp
  llvm/unittests/IR/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/IR/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/IR/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,129 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy = FunctionType::get(Type::getVoidTy(Ctx),
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 6U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 6U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, DbgLoc) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+
+  IRBuilder<> Builder(BB);
+
+  DIBuilder DIB(*M);
+  auto File = DIB.createFile("test.dbg", "/");
+  auto CU =
+  DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+  auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+  auto SP = DIB.createFunction(
+  CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+  DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+  F->setSubprogram(SP);
+  auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+  DIB.finalize();
+
+  DebugLoc DL = DebugLoc::get(3, 7, Scope);
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  CallInst *GTID = dyn_cast(>front());
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_EQ(GTID->getDebugLoc(), DL);
+  EXPECT_EQ(Barrier->getDebugLoc(), DL);
+  EXPECT_TRUE(isa(Barrier->getOperand(0)));
+  if (!isa(Barrier->getOperand(0)))
+return;
+  GlobalVariable *Ident = cast(Barrier->getOperand(0));
+  EXPECT_TRUE(Ident->hasInitializer());
+  if (!Ident->hasInitializer())
+return;
+  Constant *Initializer = Ident->getInitializer();
+  EXPECT_TRUE(
+  isa(Initializer->getOperand(4)->stripPointerCasts()));
+  GlobalVariable *SrcStrGlob =
+  cast(Initializer->getOperand(4)->stripPointerCasts());
+  if (!SrcStrGlob)
+return;
+  EXPECT_TRUE(isa(SrcStrGlob->getInitializer()));
+  ConstantDataArray *SrcSrc =
+  dyn_cast(SrcStrGlob->getInitializer());
+  if (!SrcSrc)
+return;
+  EXPECT_EQ(SrcSrc->getAsCString(), ";test.dbg;foo;3;7;;");
+}
+} // namespace
Index: 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228328.
jdoerfert added a comment.

Remove accidental newline change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp
  llvm/lib/IR/OpenMPIRBuilder.cpp
  llvm/unittests/IR/CMakeLists.txt
  llvm/unittests/IR/IRBuilderTest.cpp
  llvm/unittests/IR/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/IR/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/IR/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,129 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy = FunctionType::get(Type::getVoidTy(Ctx),
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 6U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 6U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, DbgLoc) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+
+  IRBuilder<> Builder(BB);
+
+  DIBuilder DIB(*M);
+  auto File = DIB.createFile("test.dbg", "/");
+  auto CU =
+  DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+  auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+  auto SP = DIB.createFunction(
+  CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+  DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+  F->setSubprogram(SP);
+  auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+  DIB.finalize();
+
+  DebugLoc DL = DebugLoc::get(3, 7, Scope);
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  CallInst *GTID = dyn_cast(>front());
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_EQ(GTID->getDebugLoc(), DL);
+  EXPECT_EQ(Barrier->getDebugLoc(), DL);
+  EXPECT_TRUE(isa(Barrier->getOperand(0)));
+  if (!isa(Barrier->getOperand(0)))
+return;
+  GlobalVariable *Ident = cast(Barrier->getOperand(0));
+  EXPECT_TRUE(Ident->hasInitializer());
+  if (!Ident->hasInitializer())
+return;
+  Constant *Initializer = Ident->getInitializer();
+  EXPECT_TRUE(
+  isa(Initializer->getOperand(4)->stripPointerCasts()));
+  GlobalVariable *SrcStrGlob =
+  cast(Initializer->getOperand(4)->stripPointerCasts());
+  if (!SrcStrGlob)
+return;
+  EXPECT_TRUE(isa(SrcStrGlob->getInitializer()));
+  ConstantDataArray *SrcSrc =
+  dyn_cast(SrcStrGlob->getInitializer());
+  if (!SrcSrc)
+return;
+  EXPECT_EQ(SrcSrc->getAsCString(), ";test.dbg;foo;3;7;;");
+}
+} // namespace
Index: llvm/unittests/IR/IRBuilderTest.cpp

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228320.
jdoerfert added a comment.

Use source location information to build the ident string and debug info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp
  llvm/lib/IR/OpenMPIRBuilder.cpp
  llvm/unittests/IR/CMakeLists.txt
  llvm/unittests/IR/IRBuilderTest.cpp
  llvm/unittests/IR/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/IR/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/IR/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,129 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy = FunctionType::get(Type::getVoidTy(Ctx),
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+uninitializeTypes();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 6U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 6U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, DbgLoc) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+
+  IRBuilder<> Builder(BB);
+
+  DIBuilder DIB(*M);
+  auto File = DIB.createFile("test.dbg", "/");
+  auto CU =
+  DIB.createCompileUnit(dwarf::DW_LANG_C, File, "llvm-C", true, "", 0);
+  auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+  auto SP = DIB.createFunction(
+  CU, "foo", "", File, 1, Type, 1, DINode::FlagZero,
+  DISubprogram::SPFlagDefinition | DISubprogram::SPFlagOptimized);
+  F->setSubprogram(SP);
+  auto Scope = DIB.createLexicalBlockFile(SP, File, 0);
+  DIB.finalize();
+
+  DebugLoc DL = DebugLoc::get(3, 7, Scope);
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  CallInst *GTID = dyn_cast(>front());
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_EQ(GTID->getDebugLoc(), DL);
+  EXPECT_EQ(Barrier->getDebugLoc(), DL);
+  EXPECT_TRUE(isa(Barrier->getOperand(0)));
+  if (!isa(Barrier->getOperand(0)))
+return;
+  GlobalVariable *Ident = cast(Barrier->getOperand(0));
+  EXPECT_TRUE(Ident->hasInitializer());
+  if (!Ident->hasInitializer())
+return;
+  Constant *Initializer = Ident->getInitializer();
+  EXPECT_TRUE(
+  isa(Initializer->getOperand(4)->stripPointerCasts()));
+  GlobalVariable *SrcStrGlob =
+  cast(Initializer->getOperand(4)->stripPointerCasts());
+  if (!SrcStrGlob)
+return;
+  EXPECT_TRUE(isa(SrcStrGlob->getInitializer()));
+  ConstantDataArray *SrcSrc =
+  dyn_cast(SrcStrGlob->getInitializer());
+  if (!SrcSrc)
+return;
+  EXPECT_EQ(SrcSrc->getAsCString(), 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D69785#1737581 , @lebedev.ri wrote:

> As far as i can tell the builder does not add any debug info.
>  Should it?


It does now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

As far as i can tell the builder does not add any debug info.
Should it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228259.
jdoerfert added a comment.

Improve type generation & handling, provide examples for function types and
more attributes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp
  llvm/lib/IR/OpenMPIRBuilder.cpp
  llvm/unittests/IR/CMakeLists.txt
  llvm/unittests/IR/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/IR/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/IR/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,77 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy = FunctionType::get(Type::getVoidTy(Ctx),
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+} // namespace
Index: llvm/unittests/IR/CMakeLists.txt
===
--- llvm/unittests/IR/CMakeLists.txt
+++ llvm/unittests/IR/CMakeLists.txt
@@ -28,6 +28,7 @@
   ManglerTest.cpp
   MetadataTest.cpp
   ModuleTest.cpp
+  OpenMPIRBuilderTest.cpp
   PassManagerTest.cpp
   PatternMatch.cpp
   TimePassesTest.cpp
Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -0,0 +1,222 @@
+//===- OpenMPIRBuilder.cpp - Builder for LLVM-IR for OpenMP directives ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/IRBuilder.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
+
+using namespace llvm;
+using namespace omp;
+using namespace types;
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID,
+  bool Annotate) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228134.
jdoerfert marked 3 inline comments as done.
jdoerfert added a comment.

LLVM code only, added unit test and fix issues


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp
  llvm/lib/IR/OpenMPIRBuilder.cpp
  llvm/unittests/IR/CMakeLists.txt
  llvm/unittests/IR/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/IR/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/IR/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,77 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy = FunctionType::get(Type::getVoidTy(Ctx),
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.CreateBarrier({}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.CreateBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+} // namespace
Index: llvm/unittests/IR/CMakeLists.txt
===
--- llvm/unittests/IR/CMakeLists.txt
+++ llvm/unittests/IR/CMakeLists.txt
@@ -28,6 +28,7 @@
   ManglerTest.cpp
   MetadataTest.cpp
   ModuleTest.cpp
+  OpenMPIRBuilderTest.cpp
   PassManagerTest.cpp
   PatternMatch.cpp
   TimePassesTest.cpp
Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -0,0 +1,227 @@
+//===- OpenMPIRBuilder.cpp - Builder for LLVM-IR for OpenMP directives ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/IRBuilder.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
+
+using namespace llvm;
+using namespace omp;
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 6 inline comments as done.
jdoerfert added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.def:215
 LANGOPT(OpenMPCUDAMode, 1, 0, "Generate code for OpenMP pragmas in 
SIMT/SPMD mode")
+LANGOPT(OpenMPNewCodegen  , 1, 0, "Use the experimental OpenMP-IR-Builder 
codegen path.")
 LANGOPT(OpenMPCUDAForceFullRuntime , 1, 0, "Force to use full runtime in all 
constructs when offloading to CUDA devices")

Meinersbur wrote:
> Shouldn't this rather be a CODEGENOPT (`CodeGenOptions.def`)?
This can/should be changed for all relevant fopenmp options above in a 
dedicated patch. My option mimics other that exist.



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

Meinersbur wrote:
> jdoerfert wrote:
> > JonChesterfield wrote:
> > > Sharing constants between the compiler and the runtime is an interesting 
> > > subproblem. I think the cleanest solution is the one used by libc, where 
> > > the compiler generates header files containing the constants which the 
> > > runtime library includes.
> > I'd prefer not to tackle this right now but get this done first and revisit 
> > the issue later. OK?
> I don't think this is a good solution. It means that libomp cannot built 
> built anymore without also building clang. Moreover, the values cannot be 
> changed anyway since it would break the ABI.
> 
> I'd go the other route: The libomp defines what it's ABI is, the compiler 
> generates code for it. 
This patch doesn't change what we do, just where. The numbers are hard coded in 
clang now. Let's start a discussion on the list and if we come up with a 
different scheme we do it after this landed.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:120
+  if (!SrcLocStr) {
+Constant *Initializer =
+ConstantDataArray::getString(M.getContext(), LocStr);

ABataev wrote:
> `auto *`?
No benefit and less clear, I'll stick with the type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/lib/IR/OpenMPConstants.cpp:1
-//===- OpenMPConstants.cpp - Helpers related to OpenMP code generation ---===//
+//===- OpenMPIRBuilder.cpp - Builder for LLVM-IR for OpenMP directives 
===//
 //

Constants?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:120
+  if (!SrcLocStr) {
+Constant *Initializer =
+ConstantDataArray::getString(M.getContext(), LocStr);

`auto *`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 227986.
jdoerfert marked 3 inline comments as done.
jdoerfert added a comment.

LLVM stuff only + unit test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp
  llvm/lib/IR/OpenMPIRBuilder.cpp
  llvm/unittests/IR/CMakeLists.txt
  llvm/unittests/IR/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/IR/OpenMPIRBuilderTest.cpp
===
--- /dev/null
+++ llvm/unittests/IR/OpenMPIRBuilderTest.cpp
@@ -0,0 +1,77 @@
+//===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/Verifier.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace omp;
+
+namespace {
+
+class OpenMPIRBuilderTest : public testing::Test {
+protected:
+  void SetUp() override {
+M.reset(new Module("MyModule", Ctx));
+FunctionType *FTy = FunctionType::get(Type::getVoidTy(Ctx),
+  /*isVarArg=*/false);
+F = Function::Create(FTy, Function::ExternalLinkage, "", M.get());
+BB = BasicBlock::Create(Ctx, "", F);
+  }
+
+  void TearDown() override {
+BB = nullptr;
+M.reset();
+  }
+
+  LLVMContext Ctx;
+  std::unique_ptr M;
+  Function *F;
+  BasicBlock *BB;
+};
+
+TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  IRBuilder<> Builder(BB);
+
+  OMPBuilder.createBarrier({}, OMPD_for);
+  EXPECT_TRUE(M->global_empty());
+  EXPECT_EQ(M->size(), 1U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 0U);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  OMPBuilder.createBarrier(Loc, OMPD_for);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 1U);
+  EXPECT_EQ(BB->size(), 2U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+
+  CallInst *Barrier = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Barrier, nullptr);
+  EXPECT_EQ(Barrier->getNumArgOperands(), 2U);
+  EXPECT_EQ(Barrier->getCalledFunction()->getName(), "__kmpc_barrier");
+
+  EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+} // namespace
Index: llvm/unittests/IR/CMakeLists.txt
===
--- llvm/unittests/IR/CMakeLists.txt
+++ llvm/unittests/IR/CMakeLists.txt
@@ -28,6 +28,7 @@
   ManglerTest.cpp
   MetadataTest.cpp
   ModuleTest.cpp
+  OpenMPIRBuilderTest.cpp
   PassManagerTest.cpp
   PatternMatch.cpp
   TimePassesTest.cpp
Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -0,0 +1,227 @@
+//===- OpenMPIRBuilder.cpp - Builder for LLVM-IR for OpenMP directives ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/IRBuilder.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
+
+using namespace llvm;
+using namespace omp;
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.def:215
 LANGOPT(OpenMPCUDAMode, 1, 0, "Generate code for OpenMP pragmas in 
SIMT/SPMD mode")
+LANGOPT(OpenMPNewCodegen  , 1, 0, "Use the experimental OpenMP-IR-Builder 
codegen path.")
 LANGOPT(OpenMPCUDAForceFullRuntime , 1, 0, "Force to use full runtime in all 
constructs when offloading to CUDA devices")

Shouldn't this rather be a CODEGENOPT (`CodeGenOptions.def`)?



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

jdoerfert wrote:
> JonChesterfield wrote:
> > Sharing constants between the compiler and the runtime is an interesting 
> > subproblem. I think the cleanest solution is the one used by libc, where 
> > the compiler generates header files containing the constants which the 
> > runtime library includes.
> I'd prefer not to tackle this right now but get this done first and revisit 
> the issue later. OK?
I don't think this is a good solution. It means that libomp cannot built built 
anymore without also building clang. Moreover, the values cannot be changed 
anyway since it would break the ABI.

I'd go the other route: The libomp defines what it's ABI is, the compiler 
generates code for it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_new_codegen : Flag<["-"], "fopenmp-new-codegen">, Group, 
Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;

JonChesterfield wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Maybe just `-fopenmp-experimental`?
> > I would prefer the option to be self explanatory but I'm not married to the 
> > current name.
> `enable-openmpirbuilder?`
Sure, I'll change it.



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

JonChesterfield wrote:
> Sharing constants between the compiler and the runtime is an interesting 
> subproblem. I think the cleanest solution is the one used by libc, where the 
> compiler generates header files containing the constants which the runtime 
> library includes.
I'd prefer not to tackle this right now but get this done first and revisit the 
issue later. OK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_new_codegen : Flag<["-"], "fopenmp-new-codegen">, Group, 
Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;

jdoerfert wrote:
> ABataev wrote:
> > Maybe just `-fopenmp-experimental`?
> I would prefer the option to be self explanatory but I'm not married to the 
> current name.
`enable-openmpirbuilder?`



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

Sharing constants between the compiler and the runtime is an interesting 
subproblem. I think the cleanest solution is the one used by libc, where the 
compiler generates header files containing the constants which the runtime 
library includes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69785#1734292 , @jdoerfert wrote:

> In D69785#1734205 , @ABataev wrote:
>
> > Also, I think it would better to split LLVM part and clang part into 
> > separate patches.
>
>
> What do you mean exactly and why?


I just think it would be better to split this patch into 2 parts, one for LLVM 
builder (without tests, probably, or just with some kind of unittests) + 
another one for clang-related changes. It will reduce the size of the patches 
and all that stuff related to `keep patches smaller`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

FWIW, *I will enable the new pass in some tests before this goes in*


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.

In D69785#1734205 , @ABataev wrote:

> Also, I think it would better to split LLVM part and clang part into separate 
> patches.


What do you mean exactly and why?




Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:29
+  /// not have an effect on \p M (see initialize).
+  OpenMPIRBuilder(Module ) : M(M), Builder(M.getContext()) {}
+

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Do we need a new `Builder` here or we can reuse the one from clang 
> > > CodeGenFunction?
> > If you have a "simple" way to do it, we can think about it but I am still 
> > unsure if that is actually useful. The clang (=frontend) builder is used 
> > for callbacks so user code is build with it either way. We could set up 
> > ours here differently if we wish to and I'm a little afraid we would 
> > generate some unwanted interactions.
> > 
> > That being said, I tried to reuse the one in clang but struggled *a long 
> > time* to make it work. The problem is that it is a templated class with 
> > Clang specific template parameters. We would need to make this a template 
> > class as well (I think) and that comes with a long tail of problems.
> > 
> You can make this class a template and instantiate it with the type of the 
> CodeGenFunction IRBuilder and pass it by reference in the constructor. But 
> only if it really worth it.
That doesn't work as easily because the implementation is not part of this 
header, so we need an extern template and we'll open up a link nightmare that I 
would like to avoid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Also, I think it would better to split LLVM part and clang part into separate 
patches.




Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:29
+  /// not have an effect on \p M (see initialize).
+  OpenMPIRBuilder(Module ) : M(M), Builder(M.getContext()) {}
+

jdoerfert wrote:
> ABataev wrote:
> > Do we need a new `Builder` here or we can reuse the one from clang 
> > CodeGenFunction?
> If you have a "simple" way to do it, we can think about it but I am still 
> unsure if that is actually useful. The clang (=frontend) builder is used for 
> callbacks so user code is build with it either way. We could set up ours here 
> differently if we wish to and I'm a little afraid we would generate some 
> unwanted interactions.
> 
> That being said, I tried to reuse the one in clang but struggled *a long 
> time* to make it work. The problem is that it is a templated class with Clang 
> specific template parameters. We would need to make this a template class as 
> well (I think) and that comes with a long tail of problems.
> 
You can make this class a template and instantiate it with the type of the 
CodeGenFunction IRBuilder and pass it by reference in the constructor. But only 
if it really worth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 227902.
jdoerfert added a comment.

make it a class (NFC)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- llvm/lib/IR/OpenMPIRBuilder.cpp
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -5,13 +5,20 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
 //===--===//
 
-#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/OpenMPIRBuilder.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/IRBuilder.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
 
 using namespace llvm;
 using namespace omp;
@@ -32,3 +39,206 @@
   }
   llvm_unreachable("Invalid OpenMP directive kind");
 }
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we need one.
+switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = Function::Create(FunctionType::get(ReturnType,\
+ArrayRef{__VA_ARGS__}, \
+IsVarArg), \
+  GlobalValue::ExternalLinkage, Str, M);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+assert(Fn && "Failed to create OpenMP runtime function");
+
+LLVMContext  = Fn->getContext();
+// Add attributes to the new declaration.
+switch (FnID) {
+#define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)\
+  case Enum:   \
+Fn->setAttributes( \
+AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));  \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+default:
+  // Attributes are optional.
+  break;
+}
+  }
+
+  return Fn;
+}
+
+void OpenMPIRBuilder::initialize() {
+  LLVMContext  = M.getContext();
+
+  // Create all simple and struct types exposed by the runtime and remember the
+  // llvm::PointerTypes of them for easy access later.
+  Type *T;
+#define OMP_TYPE(VarName, InitValue) this->VarName = InitValue;
+#define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
+  T = M.getTypeByName(StructName); \
+  if (!T)  \
+T = StructType::create(Ctx, {__VA_ARGS__}, StructName);\
+  this->VarName = PointerType::getUnqual(T);
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr,
+ IdentFlag LocFlags) {
+  // Enable "C-mode".
+  LocFlags |= OMP_IDENT_FLAG_KMPC;
+
+  GlobalVariable * = IdentMap[{SrcLocStr, uint64_t(LocFlags)}];
+  if (!DefaultIdent) {
+Constant *I32Null = ConstantInt::getNullValue(Int32);
+Constant *IdentData[] = {I32Null,
+ ConstantInt::get(Int32, uint64_t(LocFlags)),
+ I32Null, I32Null, SrcLocStr};
+Constant *Initializer = ConstantStruct::get(
+cast(IdentPtr->getPointerElementType()), IdentData);
+
+// Look for existing encoding of the location + flags, not needed but
+// minimizes the difference to the existing solution while we transition.
+for (GlobalVariable  : M.getGlobalList())
+  if (GV.getType() == IdentPtr && GV.hasInitializer())
+if (GV.getInitializer() == Initializer)
+  return DefaultIdent = 
+
+DefaultIdent = new GlobalVariable(M, IdentPtr->getPointerElementType(),
+

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_new_codegen : Flag<["-"], "fopenmp-new-codegen">, Group, 
Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;

ABataev wrote:
> Maybe just `-fopenmp-experimental`?
I would prefer the option to be self explanatory but I'm not married to the 
current name.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:29
+  /// not have an effect on \p M (see initialize).
+  OpenMPIRBuilder(Module ) : M(M), Builder(M.getContext()) {}
+

ABataev wrote:
> Do we need a new `Builder` here or we can reuse the one from clang 
> CodeGenFunction?
If you have a "simple" way to do it, we can think about it but I am still 
unsure if that is actually useful. The clang (=frontend) builder is used for 
callbacks so user code is build with it either way. We could set up ours here 
differently if we wish to and I'm a little afraid we would generate some 
unwanted interactions.

That being said, I tried to reuse the one in clang but struggled *a long time* 
to make it work. The problem is that it is a templated class with Clang 
specific template parameters. We would need to make this a template class as 
well (I think) and that comes with a long tail of problems.




Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:52
+  ///should be checked and acted upon.
+  void createOMPBarrier(const LocationDescription , omp::Directive DK,
+bool CheckCancelFlag = true);

ABataev wrote:
> Do you really need to spell it as `createOMPBarrier`? Maybe, just 
> `createBarrier` since it is already a member of OMPBuilder.
> Do you really need to spell it as createOMPBarrier? Maybe, just createBarrier 
> since it is already a member of OMPBuilder.

fair point. I will rename if no one objects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 227900.
jdoerfert marked 6 inline comments as done.
jdoerfert added a comment.

Minor changes according to comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- llvm/lib/IR/OpenMPIRBuilder.cpp
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -5,13 +5,20 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
 //===--===//
 
-#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/OpenMPIRBuilder.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/IRBuilder.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
 
 using namespace llvm;
 using namespace omp;
@@ -32,3 +39,206 @@
   }
   llvm_unreachable("Invalid OpenMP directive kind");
 }
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we need one.
+switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = Function::Create(FunctionType::get(ReturnType,\
+ArrayRef{__VA_ARGS__}, \
+IsVarArg), \
+  GlobalValue::ExternalLinkage, Str, M);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+assert(Fn && "Failed to create OpenMP runtime function");
+
+LLVMContext  = Fn->getContext();
+// Add attributes to the new declaration.
+switch (FnID) {
+#define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)\
+  case Enum:   \
+Fn->setAttributes( \
+AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));  \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+default:
+  // Attributes are optional.
+  break;
+}
+  }
+
+  return Fn;
+}
+
+void OpenMPIRBuilder::initialize() {
+  LLVMContext  = M.getContext();
+
+  // Create all simple and struct types exposed by the runtime and remember the
+  // llvm::PointerTypes of them for easy access later.
+  Type *T;
+#define OMP_TYPE(VarName, InitValue) this->VarName = InitValue;
+#define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
+  T = M.getTypeByName(StructName); \
+  if (!T)  \
+T = StructType::create(Ctx, {__VA_ARGS__}, StructName);\
+  this->VarName = PointerType::getUnqual(T);
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr,
+ IdentFlag LocFlags) {
+  // Enable "C-mode".
+  LocFlags |= OMP_IDENT_FLAG_KMPC;
+
+  GlobalVariable * = IdentMap[{SrcLocStr, uint64_t(LocFlags)}];
+  if (!DefaultIdent) {
+Constant *I32Null = ConstantInt::getNullValue(Int32);
+Constant *IdentData[] = {I32Null,
+ ConstantInt::get(Int32, uint64_t(LocFlags)),
+ I32Null, I32Null, SrcLocStr};
+Constant *Initializer = ConstantStruct::get(
+cast(IdentPtr->getPointerElementType()), IdentData);
+
+// Look for existing encoding of the location + flags, not needed but
+// minimizes the difference to the existing solution while we transition.
+for (GlobalVariable  : M.getGlobalList())
+  if (GV.getType() == IdentPtr && GV.hasInitializer())
+if (GV.getInitializer() == Initializer)
+  return DefaultIdent = 
+
+DefaultIdent = new GlobalVariable(M, 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_new_codegen : Flag<["-"], "fopenmp-new-codegen">, Group, 
Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;

Maybe just `-fopenmp-experimental`?



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:25
+/// Each OpenMP directive has a corresponding public generator method.
+struct OpenMPIRBuilder {
+

class?



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:29
+  /// not have an effect on \p M (see initialize).
+  OpenMPIRBuilder(Module ) : M(M), Builder(M.getContext()) {}
+

Do we need a new `Builder` here or we can reuse the one from clang 
CodeGenFunction?



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:52
+  ///should be checked and acted upon.
+  void createOMPBarrier(const LocationDescription , omp::Directive DK,
+bool CheckCancelFlag = true);

Do you really need to spell it as `createOMPBarrier`? Maybe, just 
`createBarrier` since it is already a member of OMPBuilder.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:62
+  /// Return the (LLVM-IR) string describing the source location \p LocStr.
+  Constant *getOrCreateSrcLocStr(std::string LocStr);
+

StringRef?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:178
+
+auto FnDecl = getOrCreateRuntimeFunction(OMPRTL___kmpc_global_thread_num);
+Instruction *Call =

`Function *` instead of `auto`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 227895.
jdoerfert marked an inline comment as done.
jdoerfert added a comment.

Be consistent wrt. enum classes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- llvm/lib/IR/OpenMPIRBuilder.cpp
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -5,13 +5,20 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
 //===--===//
 
-#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/OpenMPIRBuilder.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/IRBuilder.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
 
 using namespace llvm;
 using namespace omp;
@@ -32,3 +39,205 @@
   }
   llvm_unreachable("Invalid OpenMP directive kind");
 }
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we need one.
+switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = Function::Create(FunctionType::get(ReturnType,\
+ArrayRef{__VA_ARGS__}, \
+IsVarArg), \
+  GlobalValue::ExternalLinkage, Str, M);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+assert(Fn && "Failed to create OpenMP runtime function");
+
+LLVMContext  = Fn->getContext();
+// Add attributes to the new declaration.
+switch (FnID) {
+#define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)\
+  case Enum:   \
+Fn->setAttributes( \
+AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));  \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+default:
+  // Attributes are optional.
+  break;
+}
+  }
+
+  return Fn;
+}
+
+void OpenMPIRBuilder::initialize() {
+  LLVMContext  = M.getContext();
+
+  // Create all simple and struct types exposed by the runtime and remember the
+  // llvm::PointerTypes of them for easy access later.
+  Type *T;
+#define OMP_TYPE(VarName, InitValue) this->VarName = InitValue;
+#define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
+  T = M.getTypeByName(StructName); \
+  if (!T)  \
+T = StructType::create(Ctx, {__VA_ARGS__}, StructName);\
+  this->VarName = PointerType::getUnqual(T);
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr,
+ IdentFlag LocFlags) {
+  // Enable "C-mode".
+  LocFlags |= OMP_IDENT_FLAG_KMPC;
+
+  GlobalVariable * = IdentMap[{SrcLocStr, uint64_t(LocFlags)}];
+  if (!DefaultIdent) {
+Constant *I32Null = ConstantInt::getNullValue(Int32);
+Constant *IdentData[] = {I32Null,
+ ConstantInt::get(Int32, uint64_t(LocFlags)),
+ I32Null, I32Null, SrcLocStr};
+Constant *Initializer = ConstantStruct::get(
+cast(IdentPtr->getPointerElementType()), IdentData);
+
+// Look for existing encoding of the location + flags, not needed but
+// minimizes the difference to the existing solution while we transition.
+for (GlobalVariable  : M.getGlobalList())
+  if (GV.getType() == IdentPtr && GV.hasInitializer())
+if (GV.getInitializer() == Initializer)
+  return DefaultIdent = 
+
+DefaultIdent = new GlobalVariable(M, 

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 227891.
jdoerfert marked 5 inline comments as done.
jdoerfert added a comment.

Split D69853  out and changed according to 
(most) comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- llvm/lib/IR/OpenMPIRBuilder.cpp
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -5,14 +5,20 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
 //===--===//
 
-#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/OpenMPIRBuilder.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 
+#define DEBUG_TYPE "openmp-ir-builder"
+
 using namespace llvm;
 using namespace omp;
 
@@ -32,3 +38,204 @@
   }
   llvm_unreachable("Invalid OpenMP directive kind");
 }
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RTLFnKind FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we need one.
+switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = Function::Create(FunctionType::get(ReturnType,\
+ArrayRef{__VA_ARGS__}, \
+IsVarArg), \
+  GlobalValue::ExternalLinkage, Str, M);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+assert(Fn && "Failed to create OpenMP runtime function");
+
+LLVMContext  = Fn->getContext();
+// Add attributes to the new declaration.
+switch (FnID) {
+#define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)\
+  case Enum:   \
+Fn->setAttributes( \
+AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));  \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+default:
+  // Attributes are optional.
+  break;
+}
+  }
+
+  return Fn;
+}
+
+void OpenMPIRBuilder::initialize() {
+  LLVMContext  = M.getContext();
+
+  // Create all simple and struct types exposed by the runtime and remember the
+  // llvm::PointerTypes of them for easy access later.
+  Type *T;
+#define OMP_TYPE(VarName, InitValue) this->VarName = InitValue;
+#define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
+  T = M.getTypeByName(StructName); \
+  if (!T)  \
+T = StructType::create(Ctx, {__VA_ARGS__}, StructName);\
+  this->VarName = PointerType::getUnqual(T);
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr,
+ unsigned LocFlags) {
+  // Enable "C-mode".
+  LocFlags |= OMP_IDENT_FLAG_KMPC;
+
+  GlobalVariable * = IdentMap[{SrcLocStr, LocFlags}];
+  if (!DefaultIdent) {
+Constant *I32Null = ConstantInt::getNullValue(Int32);
+Constant *IdentData[] = {I32Null, ConstantInt::get(Int32, LocFlags),
+ I32Null, I32Null, SrcLocStr};
+Constant *Initializer = ConstantStruct::get(
+cast(IdentPtr->getPointerElementType()), IdentData);
+
+// Look for existing encoding of the location + flags, not needed but
+// minimizes the difference to the existing solution while we transition.
+for (GlobalVariable  : M.getGlobalList())
+  if (GV.getType() == IdentPtr && GV.hasInitializer())
+if (GV.getInitializer() == Initializer)
+  return DefaultIdent = 
+
+DefaultIdent = new GlobalVariable(M, IdentPtr->getPointerElementType(),
+

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added a comment.

In D69785#1733233 , @jdoerfert wrote:

> In D69785#1732951 , @rogfer01 wrote:
>
> > I made a small experiment lowering a `taskwait` (which is even simpler than 
> > `barrier` in "lowering" complexity). It was pretty straightforward after 
> > all.
>
>
> Nice, can you share the code? Was it based on the OpenMPIRBuilder (this one 
> or the old one) or somehow separate?


I posted the WIP D69828 .

Kind regards,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 16 inline comments as done.
jdoerfert added a comment.

In D69785#1732951 , @rogfer01 wrote:

> I made a small experiment lowering a `taskwait` (which is even simpler than 
> `barrier` in "lowering" complexity). It was pretty straightforward after all.


Nice, can you share the code? Was it based on the OpenMPIRBuilder (this one or 
the old one) or somehow separate?

> I have a few questions just to reassure I fully understand your vision here:

Sure thing.

> - the parameter `Loc` is currently unused but my understanding from the 
> comment in `OpenMPIRBuilder::getSrcLocStr` eventually we will convert a 
> `clang::SourceLocation` to a `llvm::Constant*` that represents the location 
> as used by KMP, right?

It is not unused everywhere (emitOMPBarrier uses it) but you are right that 
some uses are missing. The idea is that it will combine information about the 
location in LLVM-IR (which it does already) and the location in the source. The 
latter is in Clang a `clang::SourceLocation` but I want a frontend agnostic 
interface that takes only the values we need. That is, we add something along 
the lines of `StringRef FileName; unsigned LineNo, ColumnNo;` to the struct and 
use these to create the string that goes into KMP.

> - emitting a `taskwait` however has this ```lang=cpp if (auto *Region = 
> dyn_cast_or_null(CGF.CapturedStmtInfo)) 
> Region->emitUntiedSwitch(CGF); ``` but my understanding is that we can apply 
> the same scheme to these hooks as well, recursively, i.e.: progressively 
> convert them to calls to `OpenMPIRBuilder` (if needed under the flag 
> suggested above by Alexey). Does this make sense?

Yes. Eventually, the logic to create "untied switches" should move, then the 
logic surrounding it, ...
Though, I doubt we should start there as it will cause too much interaction 
with the internals of Clang.
Instead, we port task created (tied ones only), then add code that keeps track 
of untied tasks, then add the switches, ...

Does that makes sense?




Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:51
+  /// potentially other helpers into the underlying module. Must be called
+  /// before any other other method and only once!
+  void initialize();

fghanim wrote:
> ///before any other method and only once!
Agreed.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:72
+  bool CheckCancelFlag = true);
+
+  ///}

fghanim wrote:
> Suggestion: Rename to createOMPBarrier( ... ); - similar naming scheme to 
> IRBuilder, MDBuilder, etc.
Fine with me.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:24
+
+Function *OpenMPIRBuilder::getRuntimeFunction(RTLFnKind FnID) {
+  Function *Fn = nullptr;

fpetrogalli wrote:
> Mark this method as `const`? It doesn't seem to change any of the fields of 
> the instance.
Some methods could be made const (and I will) but most of them not (they modify 
maps and the builder).
Making them const has only little benefit but I'll add it where appropriate 
anyway. There is little benefit because we basically do not hand out a `const 
OpenMPIRBuilder` to anyone anyway.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:29-48
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  
\
+  case Enum:   
\
+Fn = M.getFunction(Str);   
\
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+

fpetrogalli wrote:
> Why not use `getorInsertFunction` directly instead of splitting the two cases?
because this way we can define attributes easily for a subset of RT 
declarations separate from the type definition. Arguably, we can merge 
everything into a big macro that defines name, type and attributes but I 
figured this is easier to maintain. (The cost is not important as we pay it 
once per function at most and the switches should be eliminated). Can be 
changed later if we determine attributes will become part of all runtime 
function declarations.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:177
+void OpenMPIRBuilder::emitBarrierImpl(const LocationDescription ,
+  DirektiveKind Kind, bool CheckCancelFlag,
+  bool ForceSimpleCall) {

fghanim wrote:
> DirectiveKind
(No need to repeat such comments, I will change the type name everywhere once I 
update ;))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli added a comment.

Hi @jdoerfert , thank you for working on this!

I have added some minor comments.

Francesco




Comment at: clang/lib/CodeGen/CodeGenModule.h:593
+  llvm::OpenMPIRBuilder () {
+assert(OMPBuilder != nullptr);
+return *OMPBuilder;

Nit: wouldn't the following be more informative? (and, also, I thought it was 
preferred style for LLVM codebase)

```
assert(OMPBuilder && "Invalid OMPBuilder instance");
```



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:27
+  /// IDs for all omp runtime library (RTL) functions.
+  enum RTLFnKind {
+#define OMP_RTL(Enum, ...) Enum,

I'd prefer use `enum class` instead of enums. If needed in some interface, it 
make it easier to see the actual type instead of a plain `unsigned`. No strong 
opinion though.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:24
+
+Function *OpenMPIRBuilder::getRuntimeFunction(RTLFnKind FnID) {
+  Function *Fn = nullptr;

Mark this method as `const`? It doesn't seem to change any of the fields of the 
instance.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:29-48
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  
\
+  case Enum:   
\
+Fn = M.getFunction(Str);   
\
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+

Why not use `getorInsertFunction` directly instead of splitting the two cases?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:85
+
+Value *OpenMPIRBuilder::getIdent(Constant *SrcLocStr, unsigned LocFlags) {
+  // Enable "C-mode".

`const` method?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:113
+
+Constant *OpenMPIRBuilder::getSrcLocStr(std::string LocStr) {
+  Constant * = SrcLocStrMap[LocStr];

`const` method?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:131
+
+Constant *OpenMPIRBuilder::getDefaultSrcLocStr() {
+  return getSrcLocStr(";unknown;unknown;0;0;;");

`const` method?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:140
+
+Value *OpenMPIRBuilder::getThreadID(Value *Ident) {
+  // TODO: It makes only so much sense to actually cache the global_thread_num

`const` method?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:168
+
+void OpenMPIRBuilder::emitOMPBarrier(const LocationDescription ,
+ DirektiveKind DK, bool CheckCancelFlag) {

`const` method?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:173
+  return emitBarrierImpl(Loc, DK, CheckCancelFlag,
+ /* ForceSimpleCall */ false);
+}

Nit!

```
/*ForceSimpleCall=*/ false);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added a comment.

Hi @jdoerfert, thanks a lot for putting this up this initial skeleton and 
providing an example with `barrier`.

I made a small experiment lowering a `taskwait` (which is even simpler than 
`barrier` in "lowering" complexity). It was pretty straightforward after all.

I have a few questions just to reassure I fully understand your vision here:

- the parameter `Loc` is currently unused but my understanding from the comment 
in `OpenMPIRBuilder::getSrcLocStr` eventually we will convert a 
`clang::SourceLocation` to a `llvm::Constant*` that represents the location as 
used by KMP, right?
- emitting a `taskwait` however has this

  if (auto *Region = dyn_cast_or_null(CGF.CapturedStmtInfo))
Region->emitUntiedSwitch(CGF);

but my understanding is that we can apply the same scheme to these hooks as 
well, recursively, i.e.: progressively convert them to calls to 
`OpenMPIRBuilder` (if needed under the flag suggested above by Alexey). Does 
this make sense?

Kind regards,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Fady Ghanim via Phabricator via cfe-commits]

fghanim added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:51
+  /// potentially other helpers into the underlying module. Must be called
+  /// before any other other method and only once!
+  void initialize();

///before any other method and only once!



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:72
+  bool CheckCancelFlag = true);
+
+  ///}

Suggestion: Rename to createOMPBarrier( ... ); - similar naming scheme to 
IRBuilder, MDBuilder, etc.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:169
+void OpenMPIRBuilder::emitOMPBarrier(const LocationDescription ,
+ DirektiveKind DK, bool CheckCancelFlag) {
+  assert(Loc.IP.getBlock() && "No insertion point provided!");

DirectiveKind



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:177
+void OpenMPIRBuilder::emitBarrierImpl(const LocationDescription ,
+  DirektiveKind Kind, bool CheckCancelFlag,
+  bool ForceSimpleCall) {

DirectiveKind


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 7 inline comments as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:33
+  /// IDs for all OpenMP directives.
+  enum DirektiveKind {
+#define OMP_DIRECTIVE(Enum, ...) Enum,

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > 1. `DirectiveKind`
> > > 2. Is it possible to merge these 2 types in LLVM and in clang into 1?
> > 1. Agreed.
> > 2. Yes! Clang needs to eventually use the one here. I can already make that 
> > change now or we wait a bit longer and have both coexist. The change would 
> > probably be something along the lines of: `using OpenMPDirectiveKind = 
> > llvm::OpenMPIRBuilder::DirectiveKind` in clang instead of the enum 
> > definition. We probably also want to include `llvm/IR/OpenMPKinds.def` 
> > where `OPENMP_DIRECTIVE_EXT` is now used. 
> Maybe introduce `llvm/IR/OpenMPKinds.def` and make clang use it in a separate 
> NFC patch?
Working on it already :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:33
+  /// IDs for all OpenMP directives.
+  enum DirektiveKind {
+#define OMP_DIRECTIVE(Enum, ...) Enum,

jdoerfert wrote:
> ABataev wrote:
> > 1. `DirectiveKind`
> > 2. Is it possible to merge these 2 types in LLVM and in clang into 1?
> 1. Agreed.
> 2. Yes! Clang needs to eventually use the one here. I can already make that 
> change now or we wait a bit longer and have both coexist. The change would 
> probably be something along the lines of: `using OpenMPDirectiveKind = 
> llvm::OpenMPIRBuilder::DirectiveKind` in clang instead of the enum 
> definition. We probably also want to include `llvm/IR/OpenMPKinds.def` where 
> `OPENMP_DIRECTIVE_EXT` is now used. 
Maybe introduce `llvm/IR/OpenMPKinds.def` and make clang use it in a separate 
NFC patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 12 inline comments as done.
jdoerfert added inline comments.



Comment at: clang/include/clang/Basic/OpenMPKinds.h:23-24
 enum OpenMPDirectiveKind {
-#define OPENMP_DIRECTIVE(Name) \
-  OMPD_##Name,
 #define OPENMP_DIRECTIVE_EXT(Name, Str) \

ABataev wrote:
> Better to make in a separate NFC patch.
Agreed, will do.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3483
+  auto *OMPRegionInfo = 
dyn_cast_or_null(CGF.CapturedStmtInfo);
+  if (!EmitChecks || ForceSimpleCall || !OMPRegionInfo ||
+  !OMPRegionInfo->hasCancel()) {

ABataev wrote:
> I would add an option, something like `-fopenmp-experimental` for all changes 
> with OpenMPBulder unless it at least matches in functionality/performance 
> with the existing solution.
We need the experimental flag eventually so I can add it here.

FWIW, there is only one functional difference for barriers and I will actually 
use this as a first task of the OpenMP-aware middle-end optimization. A new 
patch will be put for review shortly and once accepted we get better 
`om_get_thread_num` call merging than we could have in the front-end anyway.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3493
 return;
+
   // Build call __kmpc_cancel_barrier(loc, thread_id);

ABataev wrote:
> Better to remove all these formatting changes from the patch.
Sure.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:33
+  /// IDs for all OpenMP directives.
+  enum DirektiveKind {
+#define OMP_DIRECTIVE(Enum, ...) Enum,

ABataev wrote:
> 1. `DirectiveKind`
> 2. Is it possible to merge these 2 types in LLVM and in clang into 1?
1. Agreed.
2. Yes! Clang needs to eventually use the one here. I can already make that 
change now or we wait a bit longer and have both coexist. The change would 
probably be something along the lines of: `using OpenMPDirectiveKind = 
llvm::OpenMPIRBuilder::DirectiveKind` in clang instead of the enum definition. 
We probably also want to include `llvm/IR/OpenMPKinds.def` where 
`OPENMP_DIRECTIVE_EXT` is now used. 



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:108
+DefaultIdent->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+DefaultIdent->setAlignment(Align(8));
+  }

ABataev wrote:
> Is this correct for 32-bit platforms? Maybe rely on the frontend when 
> creating structures and all other required feature things?
None of these properties, e.g., the alignment, should ever be wrong. It might 
differ from what we have now on a certain platform but it should not be wrong 
or incompatible. That being said, we can certainly think about asking the 
frontend to generate global declarations for us (types should be the same). I 
don't see an immediate benefit of doing so but if we have a reason it is 
certainly doable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/OpenMPKinds.h:23-24
 enum OpenMPDirectiveKind {
-#define OPENMP_DIRECTIVE(Name) \
-  OMPD_##Name,
 #define OPENMP_DIRECTIVE_EXT(Name, Str) \

Better to make in a separate NFC patch.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3483
+  auto *OMPRegionInfo = 
dyn_cast_or_null(CGF.CapturedStmtInfo);
+  if (!EmitChecks || ForceSimpleCall || !OMPRegionInfo ||
+  !OMPRegionInfo->hasCancel()) {

I would add an option, something like `-fopenmp-experimental` for all changes 
with OpenMPBulder unless it at least matches in functionality/performance with 
the existing solution.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3486
+CGF.CGM.getOpenMPIRBuilder().emitOMPBarrier(
+{CGF.Builder.saveIP()}, llvm::OpenMPIRBuilder::DirektiveKind(Kind),
+EmitChecks);

`DirectiveKind`



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3493
 return;
+
   // Build call __kmpc_cancel_barrier(loc, thread_id);

Better to remove all these formatting changes from the patch.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:33
+  /// IDs for all OpenMP directives.
+  enum DirektiveKind {
+#define OMP_DIRECTIVE(Enum, ...) Enum,

1. `DirectiveKind`
2. Is it possible to merge these 2 types in LLVM and in clang into 1?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:108
+DefaultIdent->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+DefaultIdent->setAlignment(Align(8));
+  }

Is this correct for 32-bit platforms? Maybe rely on the frontend when creating 
structures and all other required feature things?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 5 inline comments as done.
jdoerfert added inline comments.



Comment at: clang/include/clang/Basic/OpenMPKinds.h:29
-  OMPD_unknown
+  OMPD_unknown,
 };
 

These changes do not impact Clang nor do they remove functionality (if 
OPENMP_DIRECTIVE is defined it will work as it did before) but they are 
necessary to make the enum values in Clang and the OpenMPIRBuilder equal.

The reason is that the OPENMP_DIRECTIVE and OPENMP_DIRECTIVE_EXT definitions in 
`OpenMPKinds.def` are interleaved but for the OpenMPIRBuilder I was hoping not 
to separate them logically. So the first three changes adjust Clang towards 
this simpler model that is still as powerful as the old one was but which 
changes the order of the enums.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3490
+  }
+
   if (!CGF.HaveInsertPoint())

The first extension could be for Clang to communicate the cancellation blocks 
to the OMPBuilder which would allow us there to handle all barriers. Note: 
There is no need for a callback (in the long run) as cancellation points will 
be handled by the OMPBuilder eventually.



Comment at: clang/test/OpenMP/for_firstprivate_codegen.cpp:291
 // CHECK: define internal void [[TMAIN_MICROTASK]](i{{[0-9]+}}* noalias 
[[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i32* dereferenceable(4) 
%{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [2 x [[S_INT_TY]]]* 
dereferenceable(8) %{{.+}}, [[S_INT_TY]]* dereferenceable(4) %{{.+}})
+// CHECK: [[GTID_CALL:%.+]] = call i32 @__kmpc_global_thread_num
 // Skip temp vars for loop

All test changes are because we do not check if there is an existing global 
thread ID value available through an argument. See the comment in 
`OpenMPIRBuilder::getThreadID` for further information and a proper way out.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:36
+#include "llvm/IR/OpenMPKinds.def"
+  };
+

The enum values are equivalent to the ones in Clangs `OpenMPDirectiveKind`, 
they have to be as we convert the latter to the former right now. At some point 
we won't need the latter but for now we should probably put a static assert 
here to verify the first and last enum values are equal.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:58
+IRBuilder<>::InsertPoint IP;
+  };
+

This struct needs to be extended with source location information in a way not 
tied to Clang. We wan to use that for two purposes: (1) generate `ident_t` with 
actual source locations embedded and (2) tell the IRBuilder to emit debug info 
if requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, 
gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim.
Herald added subscribers: cfe-commits, jfb, guansong, bollu, hiraditya, mgorny.
Herald added projects: clang, LLVM.

This is the initial patch for the OpenMP-IR-Builder, as discussed on the
mailing list ([1] and later) and at the US Dev Meeting'19.

The design is similar to D61953  but:

- placed in `llvm/lib/IR/` next to IRBuilder, for lack of a better location.
- in a non-WIP status, with proper documentation and working.
- using a OpenMPKinds.def file to manage lists of directives, runtime 
functions, types, ..., similar to the current Clang implementation.
- restricted to handle only (simple) barriers, to implement most `#pragma omp 
barrier` directives and most implicit barriers.
- properly hooked into Clang to be used if possible.
- compatible with the remaining code generation.

The plan is to have multiple people working on moving logic from Clang
here once the initial scaffolding (=this patch) landed.

[1] 
http://lists.flang-compiler.org/pipermail/flang-dev_lists.flang-compiler.org/2019-May/000197.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69785

Files:
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/OpenMP/for_firstprivate_codegen.cpp
  clang/test/OpenMP/for_linear_codegen.cpp
  clang/test/OpenMP/parallel_copyin_codegen.cpp
  clang/test/OpenMP/sections_codegen.cpp
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -0,0 +1,217 @@
+//===- OpenMPIRBuilder.cpp - Builder for LLVM-IR for OpenMP directives ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
+//===--===//
+
+#include "llvm/IR/OpenMPIRBuilder.h"
+
+#include "llvm/IR/MDBuilder.h"
+#include "llvm/Transforms/Utils/BasicBlockUtils.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
+
+using namespace llvm;
+
+Function *OpenMPIRBuilder::getRuntimeFunction(RTLFnKind FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we need one.
+switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = Function::Create(FunctionType::get(ReturnType,\
+ArrayRef{__VA_ARGS__}, \
+IsVarArg), \
+  GlobalValue::ExternalLinkage, Str, M);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+assert(Fn && "Failed to create OpenMP runtime function");
+
+LLVMContext  = Fn->getContext();
+// Add attributes to the new declaration.
+switch (FnID) {
+#define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)\
+  case Enum:   \
+Fn->setAttributes( \
+AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));  \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+default:
+  // Attributes are optional.
+  break;
+}
+  }
+
+  return Fn;
+}
+
+void OpenMPIRBuilder::initialize() {
+  LLVMContext  = M.getContext();
+
+  // Create all simple and struct types exposed by the runtime and remember the
+  // llvm::PointerTypes of them for easy access later.
+  Type *T;
+#define OMP_TYPE(VarName, InitValue) this->VarName = InitValue;
+#define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
+  T =