[PATCH] D48369: [CodeGen] Make block removal order deterministic in CodeGenPrepare

[David Stenberg via Phabricator via llvm-commits]

dstenb created this revision.
dstenb added reviewers: void, dexonsmith, spatel, skatkov.
Herald added subscribers: llvm-commits, mgrang.

Replace use of a SmallPtrSet with a SmallSetVector to make the worklist
iteration order deterministic. This is done as the order the blocks are
removed may affect whether or not PHI nodes in successor blocks are
removed.

For example, consider the following case where %bb1 and %bb2 are
removed:

  bb1:
    br i1 undef, label %bb3, label %bb4
  bb2:
    br i1 undef, label %bb4, label %bb3
  bb3:
    pv1 = phi type [ undef, %bb1 ], [ undef, %bb2], [ v0, %other ]
    br label %bb4
  bb4:
    pv2 = phi type [ undef, %bb1 ], [ undef, %bb2 ],
                   [ pv1, %bb3 ], [ v0, %other ]

If %bb2 is removed before %bb1, the incoming values from %bb1 and %bb2
to pv1 will be removed before %bb1 is removed as a predecessor to %bb4.
The pv1 node will thus be optimized out (to v0) at the time %bb1 is
removed as a predecessor to %bb4, leaving the blocks as following when
the incoming value from %bb1 has been removed:

  bb3:
    ; pv1 optimized out, incoming value to pv2 is v0
    br label %bb4
  bb4:
    pv2 = phi type [ v0, %bb3 ], [ v0, %other ]

The pv2 PHI node will be optimized away by removePredecessor() as all
incoming values are identical.

In case %bb2 is removed after %bb1, pv1 will not be optimized out at the
time %bb2 is removed as a predecessor to %bb4, leaving the blocks as
following when the incoming value from %bb2 to pv2 has been removed:

  bb3:
    pv1 = phi type [ undef, %bb2 ], [ v0, %other ]
    br label %bb4
  bb4:
    pv2 = phi type [ pv1, %bb3 ], [ v0, %other ]

The pv2 PHI node will thus not be removed in this case, ultimately
leading to the following output:

  bb3:
    ; pv1 optimized out, incoming value to pv2 is v0
    br label %bb4
  bb4:
    pv2 = phi type [ v0, %bb3 ], [ v0, %other ]

I have not looked into changing DeleteDeadBlock() so that the redundant
PHI nodes are removed.

I have not added a test case, as I was not able to create a particularly
small and (not messy) reproducer. This is likely due to SmallPtrSet
behaving deterministically when in small mode.


Repository:
  rL LLVM

https://reviews.llvm.org/D48369

Files:
  lib/CodeGen/CodeGenPrepare.cpp


Index: lib/CodeGen/CodeGenPrepare.cpp
===================================================================
--- lib/CodeGen/CodeGenPrepare.cpp
+++ lib/CodeGen/CodeGenPrepare.cpp
@@ -462,7 +462,10 @@
 
   if (!DisableBranchOpts) {
     MadeChange = false;
-    SmallPtrSet<BasicBlock*, 8> WorkList;
+    // Use a set vector to get deterministic iteration order. The order the
+    // blocks are removed may affect whether or not PHI nodes in successors
+    // are removed.
+    SmallSetVector<BasicBlock*, 8> WorkList;
     for (BasicBlock &BB : F) {
       SmallVector<BasicBlock *, 2> Successors(succ_begin(&BB), succ_end(&BB));
       MadeChange |= ConstantFoldTerminator(&BB, true);
@@ -478,7 +481,7 @@
     MadeChange |= !WorkList.empty();
     while (!WorkList.empty()) {
       BasicBlock *BB = *WorkList.begin();
-      WorkList.erase(BB);
+      WorkList.remove(BB);
       SmallVector<BasicBlock*, 2> Successors(succ_begin(BB), succ_end(BB));
 
       DeleteDeadBlock(BB);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D48369.152089.patch
Type: text/x-patch
Size: 981 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180620/f8f54875/attachment.bin>