This is to fix PR39658 where duplicated successors (and phi operands) causing miscompile.
It is just wrong. In PR39658, we are dealing with the following situation:
A: sub x, y jg C jmp B B: sub x, y jmp C C: Phi [v1, A], [v2, B]
You are trying to merge A and B together, but they cannot be merged specifically for the reason that it is actually important from which block we came to C. The value of the Phi node depends on this. You cannot just say that we no longer come from B and therefore Phi never takes value v2. It does take this value if x was less or equal than y.
This patch is not helping the ogirinal miscompile. I think that the right solution is to prohibit merging blocks A and B if there is a Phi which has different incoming values for these blocks.
Please use FileCheck to assert that the contents of Phis in question is correct.
This is the updated version. We will bail out the optimization is there are PHIs preventing the merge.
@Max: could you tell you real test case to see if this fixes the problem.
Note that this patch still leave this optimization off by default. I will have separated patch to turn if on later.
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.
A Phi cannot be the last instruction in a block, so MI != ME is redundant.
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:
your comments make a lot of sense. I would prefer to use the conservative approach:
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.