[PATCH] D27518: Moving isComplex decision to TTI

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

I have some comments on the patch, but on a higher level, I'm not sure this is the right way to do this.
The only place that'll call this new TTI hook will be isLikelyComplexAddressComputation(), and the only thing we do with the result of isLikelyComplexAddressComputation() is feed it into a different TTI hook - TTI.getAddressComputationCost().

Perhaps we can enhance getAddressComputationCost() instead of adding another hook?



================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:210
+  /// \p Stride holds a pointer to the Stride value in bytes if it is a compile
+  ///     time constant, or nullptr otherwise.
+  bool isComplexStridedAddressComputation(const APInt* Stride) const;
----------------
Extra space.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:211
+  ///     time constant, or nullptr otherwise.
+  bool isComplexStridedAddressComputation(const APInt* Stride) const;
+
----------------
Maybe "Expensive" makes more sense than "Complex" here?


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1480
+  // compile time constant stride we dont see any significant address  
+  // computation cost. 
+  return false;
----------------
What do you mean by "we don't see any significant address computation cost"? Can you give an example?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6618
   if (!C)
-    return true;
-
-  const APInt &APStepVal = C->getAPInt();
-
-  // Huge step value - give up.
-  if (APStepVal.getBitWidth() > 64)
-    return true;
+    return TTI.isComplexStridedAddressComputation(NULL);
 
----------------
NULL -> nullptr
Also, this looks a bit dirty. Maybe use Optional<>?


https://reviews.llvm.org/D27518