[PATCH] D34162: [InstCombine] Set correct insertion point for selects generated while folding phis

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In https://reviews.llvm.org/D34162#782438, @spatel wrote:

> I don't think we even need the cmp:
>
>   define <2 x i8> @pull_vector_select_constants_through_phi(i1 %cond1, i1 %cond2, <2 x i1> %x, <2 x i8> %y, <2 x i8> %z) {
>   entry:
>     br i1 %cond1, label %if1, label %else
>  
>   if1:
>     br i1 %cond2, label %if2, label %else
>  
>   if2:
>     br label %else
>  
>   else:
>     %phi = phi <2 x i1> [ %x, %if2 ], [ <i1 0, i1 1>, %entry ], [ <i1 1, i1 0>, %if1 ]
>     %sel = select <2 x i1> %phi, <2 x i8> %y, <2 x i8> %z
>     ret <2 x i8> %sel
>   }
>  
>
>
> This test actually raises a question about this transform independent of the bug. The transform can increase the number of IR instructions - is that expected?


Thanks @spatel , this test showcases the bug as well  (also, there is an increase in IR, which is expected because we now have non-constant phi operands). Regarding the transform itself, I *think* the increase in IR can be avoided for non-constant vectors, i.e. we should handle folding of PhiOperands only for constant vectors.

Until https://reviews.llvm.org/rL298845, there was incorrect code gen for the vector constants being  handled without a select. The transform was originally written such that the phi node has exactly one non-constant operand. However, this check was different in the bailout condition versus the actual IR transform. When computing the single non-constant block which feeds to the phi, we check for a *constant*. When transforming the IR, we look for *constant int*. This difference contributes the increase in IR. I'm guessing the increase was not expected, since the code was originally miscompiling for vector constants.

Should we go ahead with this fix, and come up with cost model for vector constants?


https://reviews.llvm.org/D34162