[PATCH] D27783: [MachineBlockPlacement] Don't make blocks "uneditable"

[Kyle Butt via Phabricator via llvm-commits]

iteratee added a comment.

I'm OK with the debugging, but I think it's a little over-engineered. Basically you're keeping a list of all the blocks that you think have unanalyzable fallthrough and then you want to assert if during updateTerminators you encounter a block with unanalyzable fallthrough that is not on that list. Is that correct? If so simply keeping that set and testing for membership is sufficient, and simpler.

I'm also OK removing the teeth in TailDuplicator. It bothers me that analyzeBranch has that flag at all.

My real surprise moment is that analyzeBranch would choose to modify a branch that it claims it can't analyze. Really! I'm not convinced that's correct, but I'm fine with some defensive programming in response to it.



================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:179
+  /// blocks and their successors through the pass.
+  SmallPtrSet<MachineBasicBlock *, 4> BlocksWithUnanalyzableExits;
+#endif
----------------
I think this should just be a list global to the pass. Each unanalyzable fallthrough gets merged exactly once.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:217
+  /// sequence is not analyzable.
+  bool isAnalyzableBlockExitRequired(MachineBasicBlock *MBB) const {
+    return !BlocksWithUnanalyzableExits.count(MBB);
----------------
I don't really like this function. This is basically all blocks without unanalyzable fallthrough. I'd rather just test for membership at the point where we need it.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1632
                    << "\n");
-      Chain->merge(NextBB, nullptr);
+      Chain->merge(NextBB, nullptr, /*UnanalyzableEdge=*/true);
       FI = NextFI;
----------------
Rather than polluting the merge api, BB is available right here, just mark it.


https://reviews.llvm.org/D27783