[PATCH] D14122: [SimplifyCFG] Trim duplicate basic blocks in switch cases

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/llvm/IR/BasicBlock.h:318
@@ -317,1 +317,3 @@
 
+  /// Checks if the bodies of two basic blocks are identical.
+  bool isIdenticalTo(const BasicBlock &Other) const;
----------------
Prefix the sentence with a `\brief` like in the other methods.

================
Comment at: lib/IR/BasicBlock.cpp:453
@@ +452,3 @@
+  assert(Cur1 == this->end() && Cur2 == Other.end() &&
+      "we should've handled this earlier");
+
----------------
Nit: spacing.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3411
@@ +3410,3 @@
+static bool EliminateDuplicateSwitchSuccessors(SwitchInst *SI, AssumptionCache *AC,
+                                               const DataLayout &DL) {
+
----------------
Why do you need `AC` and `DL`?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3462
@@ -3418,3 +3461,3 @@
   BasicBlock::iterator I = Succ->begin();
   while (PHINode *PHI = dyn_cast<PHINode>(I++)) {
     int Idx = PHI->getBasicBlockIndex(BB);
----------------
dylanmckay wrote:
> Does it really have a new incoming block? The block that contains the switch statement will still be the successor because all case statements are apart of the same basic block?
> 
> It is highly likely that I am misunderstanding, however.
PHI nodes have to have an entry for each incoming edge -- if multiple
edges come from the same predecessor then you need multiple entries in
the PHI node for that predecessor (all of which should have the same
incoming value).

So if you start with

```
declare void @foo()

define void @main(i1 %c) {
 entry:
  br i1 %c, label %match_case2, label %sw

 sw:
  %value = alloca i32
  store i32 5, i32* %value
  %val = load i32, i32* %value
  
  switch i32 %val, label %join [
    i32 1, label %match_case
    i32 48, label %match_case1
    i32 92, label %match_case2
    i32 23, label %match_case3
  ]

match_case:
  call void @foo()
  unreachable

match_case1:
  call void @foo()
  unreachable

match_case2:
  %x = phi i32 [ 10, %entry ], [ 20, %sw ]
  call void @foo()
  unreachable

match_case3:
  call void @foo()
  unreachable

join:
  ret void
}
```

transforming it to


```
declare void @foo()

define void @main(i1 %c) {
 entry:
  br i1 %c, label %match_case2, label %sw

 sw:
  %value = alloca i32
  store i32 5, i32* %value
  %val = load i32, i32* %value
  
  switch i32 %val, label %join [
    i32 1, label %match_case
    i32 48, label %match_case1
    i32 92, label %match_case2
    i32 23, label %match_case2 ;; change here
  ]

match_case:
  call void @foo()
  unreachable

match_case1:
  call void @foo()
  unreachable

match_case2:
  %x = phi i32 [ 10, %entry ], [ 20, %sw ]
  call void @foo()
  unreachable

match_case3:
  call void @foo()
  unreachable

join:
  ret void
}
```

is incorrect (i.e. you'll fail verification) -- you also need to update `%x` to be `phi i32 [ 10, %entry ], [ 20, %sw ], [ 20, %sw ]`.


http://reviews.llvm.org/D14122