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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


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


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:77
+             "guard-widening-widen-frequent-branches is set to true"),
+    cl::init(1000));
+
----------------
reames wrote:
> 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.
I'm using `(N-1)/N`. Will update the comment to make it clear.


================
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();
----------------
reames wrote:
> Better to move this check into the caller.
No, it is here because we should not remove `br` instructions. We don't want the caller to know about possible types of guards.


https://reviews.llvm.org/D49974