[PATCH] D148514: [BranchFolding] Remove redundant conditional branch.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:1679
+    // fallthrough or jump to us
+    if (PriorTBB && PriorTBB != MBB && (!PriorFBB || PriorFBB == MBB)) {
+      // and our block is branch only and has the same conditional branch
----------------
skatkov wrote:
> goldstein.w.n wrote:
> > There is more than just this 1 case no?
> > 
> > IIUC you have
> > ```
> > BB1 Cond0
> >         T -> Narian
> >         F -> BB2
> > 
> > BB2 Cond0
> >        T -> Narnia
> >        F -> BB3
> > ---
> > BB1 Cond0
> >       T -> Narnia
> >       F -> BB2
> > BB2 -> BB3
> > ```
> > 
> > But you could also have:
> > 
> > 1)
> > ```
> > BB1 Cond0
> >         T -> BB2
> >         F -> Narian
> > 
> > BB2 Cond0
> >        T -> BB3
> >        F -> Narnai
> > ---
> > BB1 Cond0
> >       T -> BB2
> >       F -> Narnia
> > BB2 -> BB3
> > ```
> > 
> > 2)
> > ```
> > BB1 Cond0
> >         T -> Narian
> >         F -> BB2
> > 
> > BB2 Cond0
> >        T -> BB3
> >        F -> Narnai
> > ---
> > BB1 Cond0
> >       T -> Narnia
> >       F -> BB2
> > BB2 -> Narnia
> > ```
> > 
> > 3)
> > ```
> > BB1 Cond0
> >         T -> Narian
> >         F -> BB2
> > 
> > BB2 Cond0
> >        T -> Narnia
> >        F -> BB3
> > ---
> > BB1 Cond0
> >       T -> Narnia
> >       F -> BB2
> > BB2 -> BB3
> > ```
> > 
> > In which could it would really just be that one of `PriorTBB` or `PriorFBB` equals `MBB` then where `MBB` goes based on which.
> The cases 2-3 you added are actually covered by this patch as I see.
> The first one, when BB1 conditionally jumps to BB2 is something new which we can cover I guess...
Err I duplicates the current case as case 3. There are two cars where BB2 is true. But you should get both as you handle case 1.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:1684
+      if (CurTBB && IsBranchOnlyBlock(MBB) &&
+          AreOperandsIdentical(CurCond, PriorCond) && !MBB->hasAddressTaken() &&
+          !MBB->isEHPad()) {
----------------
skatkov wrote:
> skatkov wrote:
> > goldstein.w.n wrote:
> > > Does this take into account comparisons against memory? The pointer (operand) stays the same but value in memory is modified in BB1?
> > BB1 is a predecessor or MBB? For MBB we check that it is IsBranchOnlyBlock(MBB).
> > 
> > you mean that predecessor may contains store AFTER conditional branch and conditional branch itself may reference memory?
> > 
> and -> or
> BB1 is a predecessor or MBB? For MBB we check that it is IsBranchOnlyBlock(MBB).
> 
> you mean that predecessor may contains store AFTER conditional branch and conditional branch itself may reference memory?
> 
Yes, exactly.
If this is still in SSA then it's fine, but x86 machine instructions (and other) have `rm` modifier where operands match but results can vary



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

https://reviews.llvm.org/D148514