[PATCH] D83936: [SimplifyCFG] Do not create unneeded PR Phi in block with convergent calls

2020-07-21 Thread Max Kazantsev via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG360ab707127d: [SimplifyCFG] Do not create unneeded PR Phi in 
block with convergent calls (authored by mkazantsev).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83936

Files:
  clang/test/CodeGenOpenCL/convergent.cl
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp
  llvm/test/Transforms/SimplifyCFG/convergent.ll


Index: llvm/test/Transforms/SimplifyCFG/convergent.ll
===
--- llvm/test/Transforms/SimplifyCFG/convergent.ll
+++ llvm/test/Transforms/SimplifyCFG/convergent.ll
@@ -4,7 +4,6 @@
 
 declare void @foo() convergent
 
-; FIXME: We should not be inserting a PR Phi here.
 define i32 @test_01(i32 %a) {
 ; CHECK-LABEL: @test_01(
 ; CHECK-NEXT:  entry:
@@ -14,9 +13,8 @@
 ; CHECK-NEXT:call void @foo()
 ; CHECK-NEXT:br label [[MERGE]]
 ; CHECK:   merge:
-; CHECK-NEXT:[[COND_PR:%.*]] = phi i1 [ [[COND]], [[IF_FALSE]] ], [ true, 
[[ENTRY:%.*]] ]
 ; CHECK-NEXT:call void @foo()
-; CHECK-NEXT:br i1 [[COND_PR]], label [[EXIT:%.*]], label 
[[IF_FALSE_2:%.*]]
+; CHECK-NEXT:br i1 [[COND]], label [[EXIT:%.*]], label [[IF_FALSE_2:%.*]]
 ; CHECK:   if.false.2:
 ; CHECK-NEXT:call void @foo()
 ; CHECK-NEXT:br label [[EXIT]]
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -2233,6 +2233,12 @@
   for (Instruction &I : BB->instructionsWithoutDebug()) {
 if (Size > MaxSmallBlockSize)
   return false; // Don't clone large BB's.
+
+// Can't fold blocks that contain noduplicate or convergent calls.
+if (CallInst *CI = dyn_cast(&I))
+  if (CI->cannotDuplicate() || CI->isConvergent())
+return false;
+
 // We will delete Phis while threading, so Phis should not be accounted in
 // block's size
 if (!isa(I))
@@ -2274,13 +2280,6 @@
   if (!BlockIsSimpleEnoughToThreadThrough(BB))
 return false;
 
-  // Can't fold blocks that contain noduplicate or convergent calls.
-  if (any_of(*BB, [](const Instruction &I) {
-const CallInst *CI = dyn_cast(&I);
-return CI && (CI->cannotDuplicate() || CI->isConvergent());
-  }))
-return false;
-
   // Okay, this is a simple enough basic block.  See if any phi values are
   // constants.
   for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i) {
Index: clang/test/CodeGenOpenCL/convergent.cl
===
--- clang/test/CodeGenOpenCL/convergent.cl
+++ clang/test/CodeGenOpenCL/convergent.cl
@@ -70,10 +70,9 @@
 // CHECK-NOT: call spir_func void @g()
 // CHECK: br label %[[if_end]]
 // CHECK: [[if_end]]:
-// FIXME: SimplifyCFG is being stupid inserting this Phi. It is not supposed 
to be here.
-// CHECK:  %[[tobool_not_pr:.+]] = phi i1
+// CHECK-NOT: phi i1
 // CHECK:  tail call spir_func void @convfun() #[[attr4:.+]]
-// CHECK:  br i1 %[[tobool_not_pr]], label %[[if_end3:.+]], label 
%[[if_then2:.+]]
+// CHECK:  br i1 %[[tobool]], label %[[if_end3:.+]], label %[[if_then2:.+]]
 // CHECK: [[if_then2]]:
 // CHECK: tail call spir_func void @g()
 // CHECK:  br label %[[if_end3:.+]]


Index: llvm/test/Transforms/SimplifyCFG/convergent.ll
===
--- llvm/test/Transforms/SimplifyCFG/convergent.ll
+++ llvm/test/Transforms/SimplifyCFG/convergent.ll
@@ -4,7 +4,6 @@
 
 declare void @foo() convergent
 
-; FIXME: We should not be inserting a PR Phi here.
 define i32 @test_01(i32 %a) {
 ; CHECK-LABEL: @test_01(
 ; CHECK-NEXT:  entry:
@@ -14,9 +13,8 @@
 ; CHECK-NEXT:call void @foo()
 ; CHECK-NEXT:br label [[MERGE]]
 ; CHECK:   merge:
-; CHECK-NEXT:[[COND_PR:%.*]] = phi i1 [ [[COND]], [[IF_FALSE]] ], [ true, [[ENTRY:%.*]] ]
 ; CHECK-NEXT:call void @foo()
-; CHECK-NEXT:br i1 [[COND_PR]], label [[EXIT:%.*]], label [[IF_FALSE_2:%.*]]
+; CHECK-NEXT:br i1 [[COND]], label [[EXIT:%.*]], label [[IF_FALSE_2:%.*]]
 ; CHECK:   if.false.2:
 ; CHECK-NEXT:call void @foo()
 ; CHECK-NEXT:br label [[EXIT]]
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -2233,6 +2233,12 @@
   for (Instruction &I : BB->instructionsWithoutDebug()) {
 if (Size > MaxSmallBlockSize)
   return false; // Don't clone large BB's.
+
+// Can't fold blocks that contain noduplicate or convergent calls.
+if (CallInst *CI = dyn_cast(&I))
+  if (CI->cannotDuplicate() || CI->isConvergent())
+return false;
+
 // We will delete Phis while threading,

[Differential] D83936: [SimplifyCFG] Do not create unneeded PR Phi in block with convergent calls

2020-07-21 Thread Max Kazantsev via Phabricator via cfe-commits 
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG360ab707127d: [SimplifyCFG] Do not create unneeded PR Phi in 
block with convergent calls (authored by mkazantsev).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83936

Files:
  clang/test/CodeGenOpenCL/convergent.cl
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp
  llvm/test/Transforms/SimplifyCFG/convergent.ll


Index: llvm/test/Transforms/SimplifyCFG/convergent.ll
===
--- llvm/test/Transforms/SimplifyCFG/convergent.ll
+++ llvm/test/Transforms/SimplifyCFG/convergent.ll
@@ -4,7 +4,6 @@
 
 declare void @foo() convergent
 
-; FIXME: We should not be inserting a PR Phi here.
 define i32 @test_01(i32 %a) {
 ; CHECK-LABEL: @test_01(
 ; CHECK-NEXT:  entry:
@@ -14,9 +13,8 @@
 ; CHECK-NEXT:call void @foo()
 ; CHECK-NEXT:br label [[MERGE]]
 ; CHECK:   merge:
-; CHECK-NEXT:[[COND_PR:%.*]] = phi i1 [ [[COND]], [[IF_FALSE]] ], [ true, 
[[ENTRY:%.*]] ]
 ; CHECK-NEXT:call void @foo()
-; CHECK-NEXT:br i1 [[COND_PR]], label [[EXIT:%.*]], label 
[[IF_FALSE_2:%.*]]
+; CHECK-NEXT:br i1 [[COND]], label [[EXIT:%.*]], label [[IF_FALSE_2:%.*]]
 ; CHECK:   if.false.2:
 ; CHECK-NEXT:call void @foo()
 ; CHECK-NEXT:br label [[EXIT]]
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -2233,6 +2233,12 @@
   for (Instruction &I : BB->instructionsWithoutDebug()) {
 if (Size > MaxSmallBlockSize)
   return false; // Don't clone large BB's.
+
+// Can't fold blocks that contain noduplicate or convergent calls.
+if (CallInst *CI = dyn_cast(&I))
+  if (CI->cannotDuplicate() || CI->isConvergent())
+return false;
+
 // We will delete Phis while threading, so Phis should not be accounted in
 // block's size
 if (!isa(I))
@@ -2274,13 +2280,6 @@
   if (!BlockIsSimpleEnoughToThreadThrough(BB))
 return false;
 
-  // Can't fold blocks that contain noduplicate or convergent calls.
-  if (any_of(*BB, [](const Instruction &I) {
-const CallInst *CI = dyn_cast(&I);
-return CI && (CI->cannotDuplicate() || CI->isConvergent());
-  }))
-return false;
-
   // Okay, this is a simple enough basic block.  See if any phi values are
   // constants.
   for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i) {
Index: clang/test/CodeGenOpenCL/convergent.cl
===
--- clang/test/CodeGenOpenCL/convergent.cl
+++ clang/test/CodeGenOpenCL/convergent.cl
@@ -70,10 +70,9 @@
 // CHECK-NOT: call spir_func void @g()
 // CHECK: br label %[[if_end]]
 // CHECK: [[if_end]]:
-// FIXME: SimplifyCFG is being stupid inserting this Phi. It is not supposed 
to be here.
-// CHECK:  %[[tobool_not_pr:.+]] = phi i1
+// CHECK-NOT: phi i1
 // CHECK:  tail call spir_func void @convfun() #[[attr4:.+]]
-// CHECK:  br i1 %[[tobool_not_pr]], label %[[if_end3:.+]], label 
%[[if_then2:.+]]
+// CHECK:  br i1 %[[tobool]], label %[[if_end3:.+]], label %[[if_then2:.+]]
 // CHECK: [[if_then2]]:
 // CHECK: tail call spir_func void @g()
 // CHECK:  br label %[[if_end3:.+]]


Index: llvm/test/Transforms/SimplifyCFG/convergent.ll
===
--- llvm/test/Transforms/SimplifyCFG/convergent.ll
+++ llvm/test/Transforms/SimplifyCFG/convergent.ll
@@ -4,7 +4,6 @@
 
 declare void @foo() convergent
 
-; FIXME: We should not be inserting a PR Phi here.
 define i32 @test_01(i32 %a) {
 ; CHECK-LABEL: @test_01(
 ; CHECK-NEXT:  entry:
@@ -14,9 +13,8 @@
 ; CHECK-NEXT:call void @foo()
 ; CHECK-NEXT:br label [[MERGE]]
 ; CHECK:   merge:
-; CHECK-NEXT:[[COND_PR:%.*]] = phi i1 [ [[COND]], [[IF_FALSE]] ], [ true, [[ENTRY:%.*]] ]
 ; CHECK-NEXT:call void @foo()
-; CHECK-NEXT:br i1 [[COND_PR]], label [[EXIT:%.*]], label [[IF_FALSE_2:%.*]]
+; CHECK-NEXT:br i1 [[COND]], label [[EXIT:%.*]], label [[IF_FALSE_2:%.*]]
 ; CHECK:   if.false.2:
 ; CHECK-NEXT:call void @foo()
 ; CHECK-NEXT:br label [[EXIT]]
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -2233,6 +2233,12 @@
   for (Instruction &I : BB->instructionsWithoutDebug()) {
 if (Size > MaxSmallBlockSize)
   return false; // Don't clone large BB's.
+
+// Can't fold blocks that contain noduplicate or convergent calls.
+if (CallInst *CI = dyn_cast(&I))
+  if (CI->cannotDuplicate() || CI->is

[PATCH] D139006: [UpdateTestChecks] Match define for labels

2022-12-12 Thread Max Kazantsev via Phabricator via cfe-commits 
mkazantsev added a comment.

So now every single test needs to be regenerated? It'll create straw diff from 
nowhere...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139006

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

[PATCH] D94827: [SimplifyCFG] Require and preserve dominator tree

2021-01-18 Thread Max Kazantsev via Phabricator via cfe-commits 
mkazantsev added a comment.

This is awesome. And scary. This is scarily awesome. :)




Comment at: llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:263
 const SimplifyCFGOptions &Options) {
-  assert((!RequireAndPreserveDomTree ||
-  (DT && DT->verify(DominatorTree::VerificationLevel::Full))) &&

Why remove that? It looks useful to have (maybe over-paranoid) checks that DT 
is correct at least for the first time. I'd rather keep them for a while unless 
there is a reason to drop them now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94827

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

[PATCH] D94827: [SimplifyCFG] Require and preserve dominator tree

2021-01-18 Thread Max Kazantsev via Phabricator via cfe-commits 
mkazantsev added inline comments.



Comment at: llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:263
 const SimplifyCFGOptions &Options) {
-  assert((!RequireAndPreserveDomTree ||
-  (DT && DT->verify(DominatorTree::VerificationLevel::Full))) &&

lebedev.ri wrote:
> mkazantsev wrote:
> > Why remove that? It looks useful to have (maybe over-paranoid) checks that 
> > DT is correct at least for the first time. I'd rather keep them for a while 
> > unless there is a reason to drop them now.
> Note that this checks that the DomTree *provided by the pass manager* is 
> valid.
> Do we actually want to check that here?
This assert is meant to check that we actually pass it a DomTree immediately 
from the pass manager, and not some previously modified dom tree. It's up to 
you, but I'm a little bit paranoid when it comes to validation of analysis 
structures. It's never too late to remove redundant assertions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94827

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