[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:26
 #include "llvm/ADT/Optional.h"
+#include "llvm/Analysis/ScalarEvolutionExpander.h"
+#include "llvm/Analysis/ScalarEvolutionExpressions.h"
----------------
Why do you need the expander?


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:430
+  bool isStridedAccess(const SCEV *Ptr) {
+    return Ptr && dyn_cast<SCEVAddRecExpr>(Ptr);
+  }
----------------
This dyn_cast<> should be isa<>.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:432
+  }
+  bool isConstantStridedAccess(ScalarEvolution *SE, const SCEV *Ptr) {
+    if (!isStridedAccess(Ptr))
----------------
Missing newline between functions.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:435
+      return false;
+    const SCEVAddRecExpr *AddRec = dyn_cast<SCEVAddRecExpr>(Ptr);
+    const SCEV *Step = AddRec->getStepRecurrence(*SE);
----------------
This dyn_cast<> can be a cast<> since you already know it's a SCEVAddRecExpr.
In general, you should never use dyn_cast without checking if the result is null. If you already know the cast is going to succeed, use cast<>. If not, use dyn_cast<> and check the result.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:437
+    const SCEV *Step = AddRec->getStepRecurrence(*SE);
+    return dyn_cast<SCEVConstant>(Step);
+  }
----------------
Again, isa<>.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:439
+  }
+  bool isConstantStridedAccessLessThan(ScalarEvolution *SE, const SCEV *Ptr,
+                                       int64_t MergeDistance) {
----------------
Another missing newline.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:443
+      return false;
+    const SCEVAddRecExpr *AddRec = dyn_cast<SCEVAddRecExpr>(Ptr);
+    const SCEV *Step = AddRec->getStepRecurrence(*SE);
----------------
Again, cast.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:444
+    const SCEVAddRecExpr *AddRec = dyn_cast<SCEVAddRecExpr>(Ptr);
+    const SCEV *Step = AddRec->getStepRecurrence(*SE);
+    APInt StrideVal = dyn_cast<SCEVConstant>(Step)->getAPInt();
----------------
Why not  replace isConstantStridedAccess() with something that returns the step if it succeeds? 


================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:429
 
-  if (Ty->isVectorTy() && IsComplex)
+  if (Ty->isVectorTy() && 
+      SE && 
----------------
clang-format?


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:350
 
-  if (Ty->isVectorTy() && IsComplex)
+  if (Ty->isVectorTy() && 
+      SE &&
----------------
clang-format?


https://reviews.llvm.org/D27518