[PATCH] D27518: Moving isComplex decision to TTI

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:606
+  /// of address-computation cost 
+  struct AddressAccessInfo {
+    bool  isStrided;        /// True in case the access is strided (AddRec).
----------------
delena wrote:
> Why do you need a struct? "Value *" will contain everything.
> 
> Value *Stride;
> 
> if (Stride == nullptr) - no stride,
> dyn_cast<ConstantInt>(Stride) - answers the question isConstant()
>  
IIUC, we don't necessarily have a Value* in hand, this originates from a SCEV, right?


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1482
+  // Even in the case of (loop invariant) stride whose value is not known at 
+  // compile time, the address computation will nor incur more than one extra
+  // ADD instruction (which for now we ignore). 
----------------
nor -> not


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6782
+              const Loop *TheLoop) {
+  TargetTransformInfo::AddressAccessInfo Info;            
+  Info.isStrided = false;                                              
----------------
Extra spaces after the line.
(On the line below too.)


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6782
+              const Loop *TheLoop) {
+  TargetTransformInfo::AddressAccessInfo Info;            
+  Info.isStrided = false;                                              
----------------
mkuper wrote:
> Extra spaces after the line.
> (On the line below too.)
You don't seem to initialize the fields.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7046
 
-      // True if the memory instruction's address computation is complex.
-      bool IsComplexComputation =
-          isLikelyComplexAddressComputation(Ptr, Legal, SE, TheLoop);
+      // Get some info about the address access complexity in the loop   
+      TargetTransformInfo::AddressAccessInfo Info = 
----------------
Please elaborate about what you actually get, rather than just "some info"
(this is really "Figure out whether the access is strided and what the stride is..." or something like that).


https://reviews.llvm.org/D27518