[PATCH] D14976: Fixed a failure in cost calculation for vector GEP

[Jingyue Wu via llvm-commits]

jingyue requested changes to this revision.
jingyue added a comment.
This revision now requires changes to proceed.

LGTM with some minors


================
Comment at: ../include/llvm/Analysis/TargetTransformInfoImpl.h:421
@@ +420,3 @@
+      if (!ConstIdx)
+        if (auto Splat = getSplatValue(*I))
+          ConstIdx = dyn_cast<ConstantInt>(Splat);
----------------
delena wrote:
> jingyue wrote:
> > I don't understand why you use `getSplatValue` here. `getSplatValue` returns the element type only when all the elements in the array/vector are the same. Are you supposed to also protect the cases where the elements are different? 
> It may be a vector of constants. In this case the ConstIdx = 0.
> For structs, the index is always splat or a scalar constant.
It makes sense now. Please add a comment explaining why we need to use `getSplatValue`. Thanks! 

================
Comment at: ../lib/Analysis/VectorUtils.cpp:421
@@ -421,2 +420,3 @@
+const llvm::Value *llvm::getSplatValue(const Value *V) {
   if (auto *CV = dyn_cast<ConstantDataVector>(V))
     return CV->getSplatValue();
----------------
delena wrote:
> jingyue wrote:
> > This if-check seems unnecessary. The one at Line 424 already checks for `ConstantDataVector`. 
> Constant and ConstantDataVector are different types. In case of all-zero-vector it is "Constant".
http://llvm.org/docs/doxygen/html/Constants_8cpp_source.html#l01435

`ConstantDataVector` is a subclass of `Constant`. `Constant::getSplatValue` checks if the constant is a `ConstantDataVector` and if so invokes `ConstantDataVector::getSplatValue`. So I think it's no longer necessary to check for `ConstantDataVector` in Line 421-422. 


Repository:
  rL LLVM

http://reviews.llvm.org/D14976