[PATCH] D54593: [X86] Fix PR39658: avoid duplicated successors in condibr merge

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added a comment.
This revision now requires changes to proceed.

I confirm that this helps all original miscompile tests I have. I am OK with the approach in general, but have some comments about implementation. I think it should be done either faster and easier (with the same scope) or made less conservative.



================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:145
+    for (auto MI = MBBI->instr_begin(), ME = MBBI->instr_end();
+         MI != ME && MI->isPHI(); ++MI) {
+      bool FromMBB = false, FromRoot = false;
----------------
A Phi cannot be the last instruction in a block, so `MI != ME` is redundant.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:152
+          if (FromRoot)
+            return true;
+        } else if (PhiOperandMBB == RootMBB) {
----------------
You are just checking that a Phi has inputs from both `MBB` and `RootMBB`, and you check it for every Phi. It actually makes no sense: all Phis either have inputs from these two blocks or they don't.

This is actually a very conservative check. Even if you have these two blocks as predecessors, but the *same* value comes to this Phi from these blocks, then this particular Phi does not prevent you from folding.

So you should pick one of two approaches:
1. Conservative approach: do the check you are doing now, but for one Phi only. It makes no sense to check incoming blocks for all Phis because they all have the same set of incoming blocks. I am not familiar with MBBs well enough, but I guess it can be done by just checking if both `MBB` and `RootMBB` are in set of our blocks' predecessors.
2. General approach: do checks for every Phi, but only return `true` if different values come from `RootMBB` and `MBB` to a Phi.


https://reviews.llvm.org/D54593