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

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a subscriber: mzolotukhin.
mzolotukhin added a comment.

Hi Cong,

Please find some comments inline.

Michael


================
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)
----------------
I believe it's an independent fix from the rest of the patch. Please commit it separately.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1401
@@ -1395,3 +1400,3 @@
 
-  /// \return  information about the register usage of the loop.
-  RegisterUsage calculateRegisterUsage();
+  /// \return  information about the register usages of the loop for the given
+  /// vectorization factors.
----------------
Nitpick: redundant whitespace after `\return`

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4605
@@ +4604,3 @@
+    // For each VF calculate its register usage.
+    auto RUs = calculateRegisterUsage(VFs);
+
----------------
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.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4607
@@ +4606,3 @@
+
+    // Select the largest VF which doen't require more registers than existing
+    // ones.
----------------
Typo: doen't

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4994-4995
@@ -4948,3 +4993,4 @@
     // Ignore instructions that are never used within the loop.
-    if (!Ends.count(I)) continue;
+    if (!Ends.count(I))
+      continue;
 
----------------
Please commit such formatting fixes separately if you feel that you need them.

================
Comment at: test/Transforms/LoopVectorize/X86/vector_max_bandwidth.ll:1
@@ +1,2 @@
+; RUN: opt -loop-vectorize -vectorizer-maximize-bandwidth -mcpu=corei7-avx -debug-only=loop-vectorize -S < %s 2>&1 | FileCheck %s
+
----------------
You'll need `REQUIRES: asserts` if you scan debug dumps.


http://reviews.llvm.org/D8943