[PATCH] D21703: Scalarizer: Support scalarizing intrinsics

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:445
@@ +444,3 @@
+      Scattered[I] = scatter(&CI, CI.getOperand(I));
+      assert(Scattered[I].size() == NumElems && "mismatched call operands");
+    }
----------------
mehdi_amini wrote:
> This seems to assume that every vector operand must have the same number of elements as the returned type?
This is true for all of the current intrinsics, I can add a comment

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:447
@@ +446,3 @@
+    }
+  }
+
----------------
mehdi_amini wrote:
> Would this loop be equivalent to: 
> 
> ```
> for (auto &Operand : I->operands()) {
>   if (!Operand.getType()->isVectorTy())
>     ScalarOperands[I] = CI.getOperand(I);
>   else {
>     Scattered[I] = scatter(&CI, CI.getOperand(I));
>     assert(Scattered[I].size() == NumElems && "mismatched call operands");
>   }
> }
> ```
> 
> I'm not sure about `hasVectorInstrinsicScalarOpd` though...
Yes, the hasVectorInstrinsicScalarOpd is why this is over the index

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:453
@@ +452,3 @@
+  Res.resize(NumElems);
+  ScalarCallOps.resize(NumArgs);
+
----------------
mehdi_amini wrote:
> I think you could pass the initial size to the ctor.
> It may even be better to call `reserve()` on `ScalarCallOps` though as the next thing is clearing it.
> I'd probably use reserve on `Res` as well and use `push_back` in the loop below.
> 
That's what I originally did, but the scalar operand complicated using push_back, so now it always sets the appropriate index. I would expect such an intrinsic to fail isTriviallyScalarizable since a different vector size probably should be treated like the scalar operand index


https://reviews.llvm.org/D21703