[PATCH] D70502: Broaden the definition of a "widenable branch"

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/GuardUtils.cpp:73
 
-  Instruction *WCAnd = cast<Instruction>(WidenableBR->getCondition());
-  // Condition is only guaranteed to dominate branch
-  WCAnd->moveBefore(WidenableBR);
-  Value *OldCond = WCAnd->getOperand(0);
-  IRBuilder<> B(WCAnd);
-  WCAnd->setOperand(0, B.CreateAnd(NewCond, OldCond));
-
+  if (match(WidenableBR->getCondition(),
+            m_Intrinsic<Intrinsic::experimental_widenable_condition>())) {
----------------
reames wrote:
> apilipenko wrote:
> > reames wrote:
> > > ebrevnov wrote:
> > > > Would it be better to use parseWidenableBranch instead of repeating its logic here.
> > > Can't.  I wish we could and tried to find a way, but we need to modify the existing instructions to preserve the "one use" requirement on the widenable condition to be able to determine it's a trivially widenable branch.  
> > Yeah, a naive approach here would be to replace `wc` with `(newc && wc)`. It handles both trivial and non-trivial widenable branches. The only problem is it makes a pattern which is not recognized by parseWidenableBranch.
> > ```
> > br (c && wc), ... - widenable branch
> > =>
> > br (c && (newc && wc)), ... - not (obviously) widenable anymore
> > ```
> > 
> > Can you add a comment that we do it this way because we need to preserve the structure `(c && wc)` which is recognized by parseWidenableBranch?
> Just to close loop, Artur and I latter released we could rephrase parseWidenableBranch in terms of Uses and simplify a lot of this code.  I landed the rewrite as rG8293f7434577
Cool. That's what I assumed by asking to reuse parseWidenableBranch :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70502