[PATCH] D8943: Calculate vectorization factor using the narrowest type instead of widest type

[Cong Hou via llvm-commits]

congh marked 4 inline comments as done.
congh added a comment.

In http://reviews.llvm.org/D8943#269553, @mzolotukhin wrote:

> Hi Cong,
>
> Please find some comments inline.
>
> Michael


Thank you for the review, Michael! Please see my inline reply.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:851-853
@@ -850,4 +850,5 @@
   std::pair<int, MVT> LT = TLI->getTypeLegalizationCost(DL, SrcVTy);
+  auto VT = TLI->getValueType(DL, SrcVTy);
   int Cost = 0;
-  if (LT.second != TLI->getValueType(DL, SrcVTy).getSimpleVT() &&
+  if (VT.isSimple() && LT.second != VT.getSimpleVT() &&
       LT.second.getVectorNumElements() == NumElem)
----------------
mzolotukhin wrote:
> I believe it's an independent fix from the rest of the patch. Please commit it separately.
This is a dependent fix and without it the test case will crash. But I could commit this fix ahead of this patch.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4605
@@ +4604,3 @@
+    // For each VF calculate its register usage.
+    auto RUs = calculateRegisterUsage(VFs);
+
----------------
mzolotukhin wrote:
> I would prefer not changing signature of `calculateRegisterUsage` and build array of `RUs` here instead. I think the original interface (takes a single vectorization factor, return register usage object for it) is more intuitive that the one operating on arrays.
The change of the signature is in consideration of performance: the part that calculates  the highest number of values that are alive at each location is shared by different VFs (actually most part are shares across different VFs). If we always consider many VFs for a given loop, then this signature also makes sense. Right?


http://reviews.llvm.org/D8943