[PATCH] D51781: [InstCombine] Do not fold scalar ops over select with vector condition.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:365
 
+  // If the select condition is a vector, the operands also must be vectors.
+  if (SI.getCondition()->getType()->isVectorTy() &&
----------------
spatel wrote:
> I found this comment confusing. The select operands themselves are vectors, but the operands of those operands must also be vectors. This can only be a problem with GEPs? If that's correct, let's state that in the comment.
I tried to make it more explicit, what do you think? I *think* it only can happen for getlementptr currently, but there might be other cases ,e.g. if the code around here becomes more powerful)


================
Comment at: test/Transforms/InstCombine/select-gep.ll:141
+
+define <2 x i64*> @test5(i64* %p, <2 x i1> %cc) {
+; CHECK-LABEL: @test5(
----------------
spatel wrote:
> We could reduce this a bit I think:
> 
> ```
> define <2 x i64*> @test5(i64* %gep1, i64* %gep2, <2 x i1> %cc) {
>   %gep3 = getelementptr i64, i64* %gep1, <2 x i64> undef
>   %gep4 = getelementptr i64, i64* %gep2, <2 x i64> undef
>   %select = select <2 x i1> %cc, <2 x i64*> %gep3, <2 x i64*> %gep4
>   ret <2 x i64*> %select
> }
> 
> ```
Thanks! I also replaced the undef operand.


https://reviews.llvm.org/D51781