[PATCH] D27919: [Loop Vectorizer] Interleave vs Gather - in some cases Gather is better.

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added a comment.

Hi Elena,

I have one comment about getWideningCost; otherwise, the patch looks fine to me now. But please let Michael have one more pass at review since he's been quiet for a while. Thanks!



================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:1995-1996
+    std::pair<Instruction *, unsigned> InstOnVF = std::make_pair(I, VF);
+    if (!WideningDecisions.count(InstOnVF))
+      return (unsigned)(-1);
+    return WideningDecisions[InstOnVF].second;
----------------
This seems weird to me. All instructions are supposed to have a cost computed by the cost model. I would much rather us assert that I is in WideningDecisions. Isn't it true that if I is a load or store, we would have already computed a cost and saved it in WideningDecisions?


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:7099
+  unsigned Cost = getWideningCost(I, VF);
+  assert(Cost != (unsigned)(-1) && "Cost should be calculated at this moment");
+  return Cost;
----------------
Just assert inside getWideningCost() that "I" is present in the mapping and return the cost. The int to unsigned max conversion is unnecessary.


Repository:
  rL LLVM

https://reviews.llvm.org/D27919