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

[Rong Xu via Phabricator via llvm-commits]

xur marked an inline comment as done.
xur added inline comments.


================
Comment at: lib/Target/X86/X86CondBrFolding.cpp:152
+          if (FromRoot)
+            return true;
+        } else if (PhiOperandMBB == RootMBB) {
----------------
mkazantsev wrote:
> 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.
your comments make a lot of sense. I would prefer to use the conservative approach:
(1) I don't think the general approach will touch many more cases -- they are rare.
(2) there does not seem to existing a function to compare phi values in general. I also need to remove duplicated phi-operands after updating. I don't feel it worthy for all the code needed.

For the conservative approach, as you suggested, I still check phi, but one only. I don't think checking the blocks' predecessors is better here. There are cases that we have both MBB and RootMBB as the predecessors but there is no PHI.






https://reviews.llvm.org/D54593