[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Fady Ghanim via Phabricator via cfe-commits]

fghanim abandoned this revision.
fghanim added a comment.

The bug this revision attempted to fix has been resolved as part of patch 
D74562 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285



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

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Fady Ghanim via Phabricator via cfe-commits]

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



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:661
+AfterIP = InsertPointTy(ForkBB, ForkBB->end());
+  }
 

jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > Why do we need all of this? Can't we just *not do it* instead? This is 
> > > code complexity that we should avoid.
> > Depends. 
> > If we want to conform with the way things are done in clang; namely, not 
> > have unreachable blocks, then yes we need to do this. If not, then no, 
> > nothing needs to change. An optimization pass will be executed at some 
> > point later that should clean all that up.
> > 
> > However, we should be careful, for example, The lit test for `critical` 
> > checks that no basic blocks were generated from the rest of the body that 
> > comes after the infinite loop. So if the choice is to not conform with 
> > clang, then we should keep an eye on these lit tests, and disable such 
> > checks for the OMPBuilder.
> > If we want to conform with the way things are done in clang;
> 
> It's not like we introduce much extra code, break anything, or make the final 
> result different.
> 
> 
> >  If not, then no, nothing needs to change. An optimization pass will be 
> > executed at some point later that should clean all that up.
> 
> Let's go with that solution and keep this code here simple, less error prone, 
> and easier to manage.
> 
> 
> > However, we should be careful, for example, The lit test for critical 
> > checks that no basic blocks were generated from the rest of the body that 
> > comes after the infinite loop. So if the choice is to not conform with 
> > clang, then we should keep an eye on these lit tests, and disable such 
> > checks for the OMPBuilder.
> 
> We already do things different and that will only become more evident 
> (TRegions!). For example, to simplify this code we do *not* cache runtime 
> calls (anymore). That is we emit a new get_thread_id call every time. (We 
> know the OpenMPOpt pass will clean it up eventually.) I get that the tests 
> change and for a while we will have clang and OMPBuilder check lines. Though, 
> once the clang CG is gone there is arguably no difference anymore because the 
> OMPBuilder behavior is then the default. As soon as we have the privatization 
> parts properly hooked up we can even start running the OMPBuilder by default 
> and soon after removing clang CG parts. If anything, we should modernize the 
> clang tests as they are a constant critique point that hinders outside 
> involvement. We could start using the llvm/utils/update__checks scripts 
> for example. We could also minimize the check lines and focus on the most 
> important bits only. (I prefer the update scripts with the pending 
> extensions, e.g., D69701)
> 
In that case, This revision is not necessary. The only fix needed is the branch 
erasure/creation change in the body CallBack (a bit more on this later), all 
the rest including the tests is not necessary. The only tests needed are 
already being done by the llvm verifier, which already reports if a BB is used 
by an orphan branch sticking around, or if a BB contains more than one 
terminator.

Regarding the issue of the branch, given that our finalization and body 
callbacks are very similar across different directives (Parallel, master, 
critical), the plan as we discussed on D72304 , is to write helper 
functions/class that we could use instead. So whoever, ends up writing that 
should make sure to include the branch changes, which makes them here redundant.

So, in the interest of everyone's time, my suggestion is to abandon this 
revision entirely for now, and just make sure that the implementation of these 
helper functions takes care of this everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285



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

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:661
+AfterIP = InsertPointTy(ForkBB, ForkBB->end());
+  }
 

fghanim wrote:
> jdoerfert wrote:
> > Why do we need all of this? Can't we just *not do it* instead? This is code 
> > complexity that we should avoid.
> Depends. 
> If we want to conform with the way things are done in clang; namely, not have 
> unreachable blocks, then yes we need to do this. If not, then no, nothing 
> needs to change. An optimization pass will be executed at some point later 
> that should clean all that up.
> 
> However, we should be careful, for example, The lit test for `critical` 
> checks that no basic blocks were generated from the rest of the body that 
> comes after the infinite loop. So if the choice is to not conform with clang, 
> then we should keep an eye on these lit tests, and disable such checks for 
> the OMPBuilder.
> If we want to conform with the way things are done in clang;

It's not like we introduce much extra code, break anything, or make the final 
result different.


>  If not, then no, nothing needs to change. An optimization pass will be 
> executed at some point later that should clean all that up.

Let's go with that solution and keep this code here simple, less error prone, 
and easier to manage.


> However, we should be careful, for example, The lit test for critical checks 
> that no basic blocks were generated from the rest of the body that comes 
> after the infinite loop. So if the choice is to not conform with clang, then 
> we should keep an eye on these lit tests, and disable such checks for the 
> OMPBuilder.

We already do things different and that will only become more evident 
(TRegions!). For example, to simplify this code we do *not* cache runtime calls 
(anymore). That is we emit a new get_thread_id call every time. (We know the 
OpenMPOpt pass will clean it up eventually.) I get that the tests change and 
for a while we will have clang and OMPBuilder check lines. Though, once the 
clang CG is gone there is arguably no difference anymore because the OMPBuilder 
behavior is then the default. As soon as we have the privatization parts 
properly hooked up we can even start running the OMPBuilder by default and soon 
after removing clang CG parts. If anything, we should modernize the clang tests 
as they are a constant critique point that hinders outside involvement. We 
could start using the llvm/utils/update__checks scripts for example. We 
could also minimize the check lines and focus on the most important bits only. 
(I prefer the update scripts with the pending extensions, e.g., D69701)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285



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

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Fady Ghanim via Phabricator via cfe-commits]

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



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:661
+AfterIP = InsertPointTy(ForkBB, ForkBB->end());
+  }
 

jdoerfert wrote:
> Why do we need all of this? Can't we just *not do it* instead? This is code 
> complexity that we should avoid.
Depends. 
If we want to conform with the way things are done in clang; namely, not have 
unreachable blocks, then yes we need to do this. If not, then no, nothing needs 
to change. An optimization pass will be executed at some point later that 
should clean all that up.

However, we should be careful, for example, The lit test for `critical` checks 
that no basic blocks were generated from the rest of the body that comes after 
the infinite loop. So if the choice is to not conform with clang, then we 
should keep an eye on these lit tests, and disable such checks for the 
OMPBuilder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285



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

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:661
+AfterIP = InsertPointTy(ForkBB, ForkBB->end());
+  }
 

Why do we need all of this? Can't we just *not do it* instead? This is code 
complexity that we should avoid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285



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

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Fady Ghanim via Phabricator via cfe-commits]

fghanim updated this revision to Diff 240949.
fghanim added a comment.

- Squashing all the commits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/parallel_codegen.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -400,6 +400,78 @@
   EXPECT_EQ(ForkCI->getArgOperand(3), F->arg_begin());
 }
 
+TEST_F(OpenMPIRBuilderTest, ParallelEndless) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+  unsigned NumBodiesGenerated = 0;
+  unsigned NumPrivatizedVars = 0;
+  unsigned NumFinalizationPoints = 0;
+
+  BasicBlock *OutlinedBodyBB = nullptr;
+  auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+   BasicBlock ) {
+++NumBodiesGenerated;
+
+auto *OldBB = OutlinedBodyBB = CodeGenIP.getBlock();
+
+// Create an endless loop.
+OldBB->getTerminator()->eraseFromParent();
+BranchInst::Create(OldBB, OldBB);
+
+Builder.ClearInsertionPoint();
+  };
+
+  auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+Value , Value *) -> InsertPointTy {
+++NumPrivatizedVars;
+return CodeGenIP;
+  };
+
+  auto FiniCB = [&](InsertPointTy CodeGenIP) { ++NumFinalizationPoints; };
+
+  IRBuilder<>::InsertPoint AfterIP =
+  OMPBuilder.CreateParallel(Loc, BodyGenCB, PrivCB, FiniCB, nullptr,
+nullptr, OMP_PROC_BIND_default, false);
+
+  EXPECT_EQ(NumBodiesGenerated, 1U);
+  EXPECT_EQ(NumPrivatizedVars, 0U);
+  EXPECT_EQ(NumFinalizationPoints, 0U);
+
+  Builder.restoreIP(AfterIP);
+  Builder.CreateRetVoid();
+
+  ASSERT_NE(OutlinedBodyBB, nullptr);
+  Function *OutlinedFn = OutlinedBodyBB->getParent();
+  EXPECT_NE(F, OutlinedFn);
+  EXPECT_FALSE(verifyModule(*M));
+  EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoUnwind));
+  EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoRecurse));
+  EXPECT_TRUE(OutlinedFn->hasParamAttribute(0, Attribute::NoAlias));
+  EXPECT_TRUE(OutlinedFn->hasParamAttribute(1, Attribute::NoAlias));
+
+  EXPECT_TRUE(OutlinedFn->hasInternalLinkage());
+  EXPECT_EQ(OutlinedFn->arg_size(), 2U);
+
+  EXPECT_EQ(OutlinedFn->getNumUses(), 1U);
+  User *Usr = OutlinedFn->user_back();
+  ASSERT_TRUE(isa(Usr));
+  CallInst *ForkCI = dyn_cast(Usr->user_back());
+  ASSERT_NE(ForkCI, nullptr);
+
+  EXPECT_EQ(ForkCI->getCalledFunction()->getName(), "__kmpc_fork_call");
+  EXPECT_EQ(ForkCI->getNumArgOperands(), 3U);
+  EXPECT_TRUE(isa(ForkCI->getArgOperand(0)));
+  EXPECT_EQ(ForkCI->getArgOperand(1),
+ConstantInt::get(Type::getInt32Ty(Ctx), 0U));
+  EXPECT_EQ(ForkCI->getArgOperand(2), Usr);
+}
+
 TEST_F(OpenMPIRBuilderTest, ParallelIfCond) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -439,6 +439,18 @@
 Worklist.push_back(SuccBB);
   }
 
+  // If we didn't emit a branch to FiniBB during body generation, it means
+  // FiniBB is unreachable (e.g. while(1);). stop generating all the
+  // unreachable blocks, and remove anything we are not going to use.
+  // Check to see if PRegPreFiniBB is reachable from PRegionBodyBB.
+  bool FoundPreFiniBB = false;
+  for (auto BI : ParallelRegionBlocks) {
+if (BI == PRegPreFiniBB) {
+  FoundPreFiniBB = true;
+  break;
+}
+  }
+
   CodeExtractorAnalysisCache CEAC(*OuterFn);
   CodeExtractor Extractor(ParallelRegionBlocks, /* DominatorTree */ nullptr,
   /* AggregateArgs */ false,
@@ -564,7 +576,7 @@
 }
   }
 
-  Builder.CreateCall(RTLFn, RealArgs);
+  CallInst *ForkCall = Builder.CreateCall(RTLFn, RealArgs);
 
   LLVM_DEBUG(dbgs() << "With fork_call placed: "
 << *Builder.GetInsertBlock()->getParent() << "\n");
@@ -583,7 +595,6 @@
   if (!ElseTI) {
 CI->eraseFromParent();
   } else {
-
 // If an "if" clause was present we are now generating the serialized
 // version into the "else" branch.
 Builder.SetInsertPoint(ElseTI);
@@ -608,22 +619,46 @@
   << *Builder.GetInsertBlock()->getParent() << "\n");
   }
 
-  // Adjust the finalization stack, verify the adjustment, and call the
-  // finalize function a last time to finalize values between the pre-fini block
-  

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Fady Ghanim via Phabricator via cfe-commits]

fghanim updated this revision to Diff 240886.
fghanim added a comment.

Adding lit test to clang for testing the fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285

Files:
  clang/test/OpenMP/parallel_codegen.cpp


Index: clang/test/OpenMP/parallel_codegen.cpp
===
--- clang/test/OpenMP/parallel_codegen.cpp
+++ clang/test/OpenMP/parallel_codegen.cpp
@@ -21,11 +21,13 @@
 // CHECK-DEBUG-DAG: %struct.ident_t = type { i32, i32, i32, i32, i8* }
 // CHECK-DEBUG-DAG: [[STR:@.+]] = private unnamed_addr constant [23 x i8] 
c";unknown;unknown;0;0;;\00"
 // CHECK-DEBUG-DAG: [[DEF_LOC_2:@.+]] = private unnamed_addr global 
%struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x 
i8], [23 x i8]* [[STR]], i32 0, i32 0) }
-// CHECK-DEBUG-DAG: [[LOC1:@.+]] = private unnamed_addr constant [{{.+}} x i8] 
c";{{.*}}parallel_codegen.cpp;main;[[@LINE+22]];1;;\00"
-// CHECK-DEBUG-DAG: [[LOC2:@.+]] = private unnamed_addr constant [{{.+}} x i8] 
c";{{.*}}parallel_codegen.cpp;tmain;[[@LINE+11]];1;;\00"
+// CHECK-DEBUG-DAG: [[LOC1:@.+]] = private unnamed_addr constant [{{.+}} x i8] 
c";{{.*}}parallel_codegen.cpp;main;[[@LINE+29]];1;;\00"
+// CHECK-DEBUG-DAG: [[LOC2:@.+]] = private unnamed_addr constant [{{.+}} x i8] 
c";{{.*}}parallel_codegen.cpp;tmain;[[@LINE+13]];1;;\00"
+// CHECK-DEBUG-DAG: [[LOC3:@.+]] = private unnamed_addr constant [{{.+}} x i8] 
c";{{.*}}parallel_codegen.cpp;tmain;[[@LINE+19]];1;;\00"
 // IRBUILDER-DEBUG-DAG: %struct.ident_t = type { i32, i32, i32, i32, i8* }
-// IRBUILDER-DEBUG-DAG: [[LOC1:@.+]] = private unnamed_addr constant [{{.+}} x 
i8] c";{{.*}}parallel_codegen.cpp;main;[[@LINE+19]];0;;\00"
-// IRBUILDER-DEBUG-DAG: [[LOC2:@.+]] = private unnamed_addr constant [{{.+}} x 
i8] c";{{.*}}parallel_codegen.cpp;tmain;[[@LINE+8]];0;;\00"
+// IRBUILDER-DEBUG-DAG: [[LOC1:@.+]] = private unnamed_addr constant [{{.+}} x 
i8] c";{{.*}}parallel_codegen.cpp;main;[[@LINE+25]];0;;\00"
+// IRBUILDER-DEBUG-DAG: [[LOC2:@.+]] = private unnamed_addr constant [{{.+}} x 
i8] c";{{.*}}parallel_codegen.cpp;tmain;[[@LINE+9]];0;;\00"
+// IRBUILDER-DEBUG-DAG: [[LOC3:@.+]] = private unnamed_addr constant [{{.+}} x 
i8] c";{{.*}}parallel_codegen.cpp;tmain;[[@LINE+15]];0;;\00"
 
 template 
 void foo(T argc) {}
@@ -38,6 +40,11 @@
   foo(argc);
   chunk_t var;(void)var[0][0];
   }
+
+  if (argc[1])
+#pragma omp parallel
+   while(1);
+
   return 0;
 }
 
@@ -113,6 +120,8 @@
 // ALL:   store i8** %argc, i8*** [[ARGC_ADDR:%.+]],
 // CHECK:   call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, 
...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void 
(i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i{{64|32}})* 
[[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], 
i{{64|32}} %{{.+}})
 // IRBUILDER:   call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, 
...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void 
(i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i{{64|32}})* 
[[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], 
i{{64|32}} %{{.+}})
+// CHECK-DAG:  call {{.*}}void (%struct.ident_t*, i32, void 
(i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 
0, void (i32*, i32*, ...)* bitcast (void (i32*, i32*)* [[OMP_OUTLINED1:@.+]] to 
void (i32*, i32*, ...)*))
+// IRBUILDER-DAG:  call {{.*}}void (%struct.ident_t*, i32, void (i32*, 
i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 0, void 
(i32*, i32*, ...)* bitcast (void (i32*, i32*)* [[OMP_OUTLINED1:@.+]] to void 
(i32*, i32*, ...)*))
 // ALL:  ret i32 0
 // ALL-NEXT:  }
 // ALL-DEBUG:   define linkonce_odr i32 [[TMAIN]](i8** %argc)
@@ -128,6 +137,13 @@
 // ALL-DEBUG:  ret i32 0
 // ALL-DEBUG-NEXT:  }
 
+// IRBUILDER:   define internal {{.*}}void [[OMP_OUTLINED1]](i32* noalias 
%{{.*}}, i32* noalias %{{.*}})
+// IRBUILDER-SAME:  #[[FN_ATTRS:[0-9]+]]
+// IRBUILDER:  br label %while.body
+// IRBUILDER-NOT:  ret %{{.*}}
+// IRBUILDER:  br label %while.body
+// IRBUILDER-NOT:  ret %{{.*}}
+
 // CHECK:   define internal {{.*}}void [[OMP_OUTLINED]](i32* noalias 
%.global_tid., i32* noalias %.bound_tid., i8*** dereferenceable({{4|8}}) %argc, 
i{{64|32}}{{.*}} %{{.+}})
 // IRBUILDER:   define internal {{.*}}void [[OMP_OUTLINED]](i32* noalias 
%{{.*}}, i32* noalias %{{.*}}, i8*** [[ARGC_REF:%.*]], i{{64|32}}{{.*}} %{{.+}})
 // CHECK:   store i8*** %argc, i8 [[ARGC_PTR_ADDR:%.+]],
@@ -152,6 +168,12 @@
 // CHECK-DEBUG-NEXT:  }
 
 // ALL: define linkonce_odr {{.*}}void [[FOO1]](i8** %argc)
+// CHECK:   define internal {{.*}}void [[OMP_OUTLINED1]](i32* noalias 
%.global_tid., i32* noalias %.bound_tid.)
+// CHECK-SAME:  #[[FN_ATTRS:[0-9]+]]
+// CHECK:   

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Fady Ghanim via Phabricator via cfe-commits]

fghanim updated this revision to Diff 240451.
fghanim added a comment.

Adding a new unittest for the this fix. Thanks to JDoerfert for Writing and 
providing me with this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285

Files:
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -400,6 +400,78 @@
   EXPECT_EQ(ForkCI->getArgOperand(3), F->arg_begin());
 }
 
+TEST_F(OpenMPIRBuilderTest, ParallelEndless) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+  unsigned NumBodiesGenerated = 0;
+  unsigned NumPrivatizedVars = 0;
+  unsigned NumFinalizationPoints = 0;
+
+  BasicBlock *OutlinedBodyBB = nullptr;
+  auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+   BasicBlock ) {
+++NumBodiesGenerated;
+
+auto *OldBB = OutlinedBodyBB = CodeGenIP.getBlock();
+
+// Create an endless loop.
+OldBB->getTerminator()->eraseFromParent();
+BranchInst::Create(OldBB, OldBB);
+
+Builder.ClearInsertionPoint();
+  };
+
+  auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+Value , Value *) -> InsertPointTy {
+++NumPrivatizedVars;
+return CodeGenIP;
+  };
+
+  auto FiniCB = [&](InsertPointTy CodeGenIP) { ++NumFinalizationPoints; };
+
+  IRBuilder<>::InsertPoint AfterIP =
+  OMPBuilder.CreateParallel(Loc, BodyGenCB, PrivCB, FiniCB, nullptr,
+nullptr, OMP_PROC_BIND_default, false);
+
+  EXPECT_EQ(NumBodiesGenerated, 1U);
+  EXPECT_EQ(NumPrivatizedVars, 0U);
+  EXPECT_EQ(NumFinalizationPoints, 0U);
+
+  Builder.restoreIP(AfterIP);
+  Builder.CreateRetVoid();
+
+  ASSERT_NE(OutlinedBodyBB, nullptr);
+  Function *OutlinedFn = OutlinedBodyBB->getParent();
+  EXPECT_NE(F, OutlinedFn);
+  EXPECT_FALSE(verifyModule(*M));
+  EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoUnwind));
+  EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoRecurse));
+  EXPECT_TRUE(OutlinedFn->hasParamAttribute(0, Attribute::NoAlias));
+  EXPECT_TRUE(OutlinedFn->hasParamAttribute(1, Attribute::NoAlias));
+
+  EXPECT_TRUE(OutlinedFn->hasInternalLinkage());
+  EXPECT_EQ(OutlinedFn->arg_size(), 2U);
+
+  EXPECT_EQ(OutlinedFn->getNumUses(), 1U);
+  User *Usr = OutlinedFn->user_back();
+  ASSERT_TRUE(isa(Usr));
+  CallInst *ForkCI = dyn_cast(Usr->user_back());
+  ASSERT_NE(ForkCI, nullptr);
+
+  EXPECT_EQ(ForkCI->getCalledFunction()->getName(), "__kmpc_fork_call");
+  EXPECT_EQ(ForkCI->getNumArgOperands(), 3U);
+  EXPECT_TRUE(isa(ForkCI->getArgOperand(0)));
+  EXPECT_EQ(ForkCI->getArgOperand(1),
+ConstantInt::get(Type::getInt32Ty(Ctx), 0U));
+  EXPECT_EQ(ForkCI->getArgOperand(2), Usr);
+}
+
 TEST_F(OpenMPIRBuilderTest, ParallelIfCond) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -623,6 +623,7 @@
   auto FiniInfo = FinalizationStack.pop_back_val();
   assert(FiniInfo.DK == OMPD_parallel &&
  "Unexpected finalization stack state!");
+
   if (FoundPreFiniBB) {
 // PRegPreFiniBB is reachable. Adjust the finalization stack, verify the
 // adjustment, and call the finalize function a last time to finalize values
@@ -631,10 +632,7 @@
 (void)FiniInfo;
 
 Instruction *PreFiniTI = PRegPreFiniBB->getTerminator();
-assert(PreFiniTI->getNumSuccessors() == 1 &&
-   PreFiniTI->getSuccessor(0)->size() == 1 &&
-   isa(PreFiniTI->getSuccessor(0)->getTerminator()) &&
-   "Unexpected CFG structure!");
+assert(PreFiniTI->getNumSuccessors() == 1 && "Unexpected CFG structure!");
 
 InsertPointTy PreFiniIP(PRegPreFiniBB, PreFiniTI->getIterator());
 FiniCB(PreFiniIP);
@@ -665,7 +663,6 @@
   for (Instruction *I : ToBeDeleted)
 I->eraseFromParent();
 
-  AfterIP.getBlock()->dump();
   return AfterIP;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Fady Ghanim via Phabricator via cfe-commits]

fghanim updated this revision to Diff 239991.
fghanim added a comment.

- Cleaning up some leftover code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73285

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -439,6 +439,18 @@
 Worklist.push_back(SuccBB);
   }
 
+  // If we didn't emit a branch to FiniBB during body generation, it means
+  // FiniBB is unreachable (e.g. while(1);). stop generating all the
+  // unreachable blocks, and remove anything we are not going to use.
+  // Check to see if PRegPreFiniBB is reachable from PRegionBodyBB.
+  bool FoundPreFiniBB = false;
+  for (auto BI : ParallelRegionBlocks) {
+if (BI == PRegPreFiniBB) {
+  FoundPreFiniBB = true;
+  break;
+}
+  }
+
   CodeExtractorAnalysisCache CEAC(*OuterFn);
   CodeExtractor Extractor(ParallelRegionBlocks, /* DominatorTree */ nullptr,
   /* AggregateArgs */ false,
@@ -564,7 +576,7 @@
 }
   }
 
-  Builder.CreateCall(RTLFn, RealArgs);
+  CallInst *ForkCall = Builder.CreateCall(RTLFn, RealArgs);
 
   LLVM_DEBUG(dbgs() << "With fork_call placed: "
 << *Builder.GetInsertBlock()->getParent() << "\n");
@@ -583,7 +595,6 @@
   if (!ElseTI) {
 CI->eraseFromParent();
   } else {
-
 // If an "if" clause was present we are now generating the serialized
 // version into the "else" branch.
 Builder.SetInsertPoint(ElseTI);
@@ -608,26 +619,53 @@
   << *Builder.GetInsertBlock()->getParent() << "\n");
   }
 
-  // Adjust the finalization stack, verify the adjustment, and call the
-  // finalize function a last time to finalize values between the pre-fini block
-  // and the exit block if we left the parallel "the normal way".
+  assert(!FinalizationStack.empty() && "Unexpected finalization stack state!");
   auto FiniInfo = FinalizationStack.pop_back_val();
-  (void)FiniInfo;
   assert(FiniInfo.DK == OMPD_parallel &&
  "Unexpected finalization stack state!");
+  if (FoundPreFiniBB) {
+// PRegPreFiniBB is reachable. Adjust the finalization stack, verify the
+// adjustment, and call the finalize function a last time to finalize values
+// between the pre-fini block and the exit block if we left the parallel
+// "the normal way".
+(void)FiniInfo;
+
+Instruction *PreFiniTI = PRegPreFiniBB->getTerminator();
+assert(PreFiniTI->getNumSuccessors() == 1 &&
+   PreFiniTI->getSuccessor(0)->size() == 1 &&
+   isa(PreFiniTI->getSuccessor(0)->getTerminator()) &&
+   "Unexpected CFG structure!");
+
+InsertPointTy PreFiniIP(PRegPreFiniBB, PreFiniTI->getIterator());
+FiniCB(PreFiniIP);
+  } else {
+// PRegPreFiniBB is unreachable. remove the unreachable blocks
+// and discard the finalization callback
+llvm::SmallVector ToBeDeletedBB;
+ToBeDeletedBB.push_back(PRegPreFiniBB);
+BranchInst *BBTerminator =
+dyn_cast_or_null(PRegPreFiniBB->getTerminator());
+while (BBTerminator) {
+  assert(!BBTerminator->isConditional() &&
+ "unexpected conditional branch in unreachable blocks");
+  BasicBlock *next = BBTerminator->getSuccessor(0);
+  ToBeDeletedBB.push_back(next);
+  BBTerminator = dyn_cast_or_null(next->getTerminator());
+}
 
-  Instruction *PreFiniTI = PRegPreFiniBB->getTerminator();
-  assert(PreFiniTI->getNumSuccessors() == 1 &&
- PreFiniTI->getSuccessor(0)->size() == 1 &&
- isa(PreFiniTI->getSuccessor(0)->getTerminator()) &&
- "Unexpected CFG structure!");
+for (auto BB : ToBeDeletedBB) {
+  BB->eraseFromParent();
+}
 
-  InsertPointTy PreFiniIP(PRegPreFiniBB, PreFiniTI->getIterator());
-  FiniCB(PreFiniIP);
+BasicBlock *ForkBB = ForkCall->getParent();
+ForkBB->getTerminator()->eraseFromParent();
+AfterIP = InsertPointTy(ForkBB, ForkBB->end());
+  }
 
   for (Instruction *I : ToBeDeleted)
 I->eraseFromParent();
 
+  AfterIP.getBlock()->dump();
   return AfterIP;
 }
 
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1377,15 +1377,15 @@
   ReturnBlock = getJumpDestInCurrentScope();
 
   llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
-  CodeGenIPBB->splitBasicBlock(CodeGenIP.getPoint());
   llvm::Instruction *CodeGenIPBBTI = CodeGenIPBB->getTerminator();
-  CodeGenIPBBTI->removeFromParent();
+  CodeGenIPBBTI->eraseFromParent();
 
   Builder.SetInsertPoint(CodeGenIPBB);
 
   EmitStmt(ParallelRegionBodyStmt);
 
-  Builder.Insert(CodeGenIPBBTI);
+  if 

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

[Fady Ghanim via Phabricator via cfe-commits]

fghanim created this revision.
fghanim added a reviewer: jdoerfert.
Herald added subscribers: llvm-commits, cfe-commits, guansong, hiraditya.
Herald added projects: clang, LLVM.

In some situations (e.g. `while(1);` ) the body block(s) will not contain a 
branch to the finalization block.
In this patch, `CreateParallel` has been modified to check if the 
`PRegPreFiniBB` is reachable before
generating finalization code. If not, will remove all unreachable blocks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73285

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -439,6 +439,18 @@
 Worklist.push_back(SuccBB);
   }
 
+  // If we didn't emit a branch to FiniBB during body generation, it means
+  // FiniBB is unreachable (e.g. while(1);). stop generating all the
+  // unreachable blocks, and remove anything we are not going to use.
+  // Check to see if PRegPreFiniBB is reachable from PRegionBodyBB.
+  bool FoundPreFiniBB = false;
+  for (auto BI : ParallelRegionBlocks) {
+if (BI == PRegPreFiniBB) {
+  FoundPreFiniBB = true;
+  break;
+}
+  }
+
   CodeExtractorAnalysisCache CEAC(*OuterFn);
   CodeExtractor Extractor(ParallelRegionBlocks, /* DominatorTree */ nullptr,
   /* AggregateArgs */ false,
@@ -564,7 +576,8 @@
 }
   }
 
-  Builder.CreateCall(RTLFn, RealArgs);
+  Builder.GetInsertBlock()->dump();
+  CallInst *ForkCall = Builder.CreateCall(RTLFn, RealArgs);
 
   LLVM_DEBUG(dbgs() << "With fork_call placed: "
 << *Builder.GetInsertBlock()->getParent() << "\n");
@@ -583,7 +596,6 @@
   if (!ElseTI) {
 CI->eraseFromParent();
   } else {
-
 // If an "if" clause was present we are now generating the serialized
 // version into the "else" branch.
 Builder.SetInsertPoint(ElseTI);
@@ -608,26 +620,54 @@
   << *Builder.GetInsertBlock()->getParent() << "\n");
   }
 
-  // Adjust the finalization stack, verify the adjustment, and call the
-  // finalize function a last time to finalize values between the pre-fini block
-  // and the exit block if we left the parallel "the normal way".
+  assert(!FinalizationStack.empty() && "Unexpected finalization stack state!");
   auto FiniInfo = FinalizationStack.pop_back_val();
-  (void)FiniInfo;
   assert(FiniInfo.DK == OMPD_parallel &&
  "Unexpected finalization stack state!");
+  if (FoundPreFiniBB) {
+// PRegPreFiniBB is reachable. Adjust the finalization stack, verify the
+// adjustment, and call the finalize function a last time to finalize values
+// between the pre-fini block and the exit block if we left the parallel
+// "the normal way".
+(void)FiniInfo;
+
+Instruction *PreFiniTI = PRegPreFiniBB->getTerminator();
+assert(PreFiniTI->getNumSuccessors() == 1 &&
+   PreFiniTI->getSuccessor(0)->size() == 1 &&
+   isa(PreFiniTI->getSuccessor(0)->getTerminator()) &&
+   "Unexpected CFG structure!");
+
+InsertPointTy PreFiniIP(PRegPreFiniBB, PreFiniTI->getIterator());
+FiniCB(PreFiniIP);
+  } else {
+// PRegPreFiniBB is unreachable. remove the blocks and discard the
+// finalization callback
+llvm::SmallVector ToBeDeletedBB;
+ToBeDeletedBB.push_back(PRegPreFiniBB);
+PRegPreFiniBB->getUniquePredecessor();
+BranchInst *BBTerminator =
+dyn_cast_or_null(PRegPreFiniBB->getTerminator());
+while (BBTerminator) {
+  assert(!BBTerminator->isConditional() &&
+ "unexpected conditional branch in unreachable blocks");
+  BasicBlock *next = BBTerminator->getSuccessor(0);
+  ToBeDeletedBB.push_back(next);
+  BBTerminator = dyn_cast_or_null(next->getTerminator());
+}
 
-  Instruction *PreFiniTI = PRegPreFiniBB->getTerminator();
-  assert(PreFiniTI->getNumSuccessors() == 1 &&
- PreFiniTI->getSuccessor(0)->size() == 1 &&
- isa(PreFiniTI->getSuccessor(0)->getTerminator()) &&
- "Unexpected CFG structure!");
+for (auto BB : ToBeDeletedBB) {
+  BB->eraseFromParent();
+}
 
-  InsertPointTy PreFiniIP(PRegPreFiniBB, PreFiniTI->getIterator());
-  FiniCB(PreFiniIP);
+BasicBlock *ForkBB = ForkCall->getParent();
+ForkBB->getTerminator()->eraseFromParent();
+AfterIP = InsertPointTy(ForkBB, ForkBB->end());
+  }
 
   for (Instruction *I : ToBeDeleted)
 I->eraseFromParent();
 
+  AfterIP.getBlock()->dump();
   return AfterIP;
 }
 
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1377,15 +1377,16 @@
   ReturnBlock = getJumpDestInCurrentScope();
 
   llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
-