[PATCH] D49974: [GuardWidening] Widen guards with conditions of frequently taken dominated branches

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:70
+             "guard-widening-frequent-branch-threshold option"),
+    cl::init(true));
+
----------------
This definitely needs to default to false initially. :)


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:77
+             "guard-widening-widen-frequent-branches is set to true"),
+    cl::init(1000));
+
----------------
Er, how is a frequency 1000?   I would expect a percentage here.  Are you using N-to-1 or (N-1)/N instead?  If so, the comment on the flag needs updated.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:267
 
-  for (auto DFI = df_begin(Root), DFE = df_end(Root);
-       DFI != DFE; ++DFI) {
+  for (auto DFI = df_begin(Root), DFE = df_end(Root); DFI != DFE; ++DFI) {
     auto *BB = (*DFI)->getBlock();
----------------
Separate whitespace change please.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:377
+  } else if (BranchInst *BI = dyn_cast<BranchInst>(GuardInst)) {
+    assert(BI->isConditional() && "Must be!");
+    return BI->getCondition();
----------------
Just use conditional early return followed by unchecked cast<BranchInst>


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:390
+    GI->setArgOperand(0, NewCond);
+  } else if (BranchInst *BI = dyn_cast<BranchInst>(GuardInst)) {
+    assert(BI->isConditional() && "Must be!");
----------------
Same as above.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:408
+  // Only erase guard intrinsics. Do nothing about branches.
+  if (IntrinsicInst *GI = dyn_cast<IntrinsicInst>(GuardInst))
+    GI->eraseFromParent();
----------------
Better to move this check into the caller.


================
Comment at: test/Transforms/GuardWidening/widen-frequent-branches.ll:10
+; Check that we don't widen without branch probability.
+define void @test_01(i1 %cond_0, i1 %cond_1) {
+; CHECK-LABEL: @test_01(
----------------
Please add at least some tests for CFGs other than diamonds.  Triangles and simple early returns are also good.


https://reviews.llvm.org/D49974