[PATCH] D131132: a better fix for llvm-reduce's basicblock pass

[John Regehr via Phabricator via llvm-commits]

regehr added inline comments.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceBasicBlocks.cpp:112
+    if (&B == &Candidate)
+      goto NextBlock;
+    for (auto BD : BBsToDelete)
----------------
arsenm wrote:
> This is just continue
yeah I was going for consistency with the goto in the following code (which isn't just continue) but can do continue if you think that's cleaner


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceBasicBlocks.cpp:125
     return true;
-  auto F = BB.getParent();
-  auto it = F->begin();
-  ++it;
-  return (it == F->end()) || (*it).hasNPredecessors(1);
+  NextBlock:;
+  }
----------------
arsenm wrote:
> ; after :
the semicolon is required. if this is all too ugly I can use a boolean flag instead of the gotos. I wrote them both and thought this way is a bit cleaner but happy to do the other thing 


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceBasicBlocks.cpp:140
+      else
+        BBsToDelete.insert(&BB);
+    }
----------------
arsenm wrote:
> When doing the MIR version I found it was easier to only track the deletion set. Does this really need a set for both?
well, the existing code uses both, and I'm trying to be minimally invasive. if we want to refactor to get rid of one of them, I think we should do it as a separate patchset


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

https://reviews.llvm.org/D131132