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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:1676
+  // BB2: Fallthrough or explicit branch to BB3
+  if (!CurUnAnalyzable && !PriorUnAnalyzable && MBB->pred_size() == 1) {
+    // If the our predecessor has a conditional branch to other block and
----------------
goldstein.w.n wrote:
> Instead of only 1 pred, you could check that all predecessors share the conditions?
It makes the patch more complicated... but let's see what we can do here...


================
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
----------------
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...


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

https://reviews.llvm.org/D148514