[PATCH] D149654: [SLP] Don't cost pointers that can be folded in getPointersChainCost

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:329
   /// chain of loads or stores within same block) operations set when lowered.
+  /// \p MemOpTy is the type of the loads/stores that will ultimately use the \p
+  /// Ptrs.
----------------
Rephrase: is the type of the memory access.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:333
   getPointersChainCost(ArrayRef<const Value *> Ptrs, const Value *Base,
-                       const PointersChainInfo &Info,
+                       const PointersChainInfo &Info, Type *MemOpTy,
                        TargetCostKind CostKind = TTI::TCK_RecipThroughput
----------------
Rename: AccessTy

(This matches the naming convention we use elsewhere for this concept.)


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfoImpl.h:1066
+        // addressing mode, then we don't need an extra ADD operation
+        unsigned Stride = DL.getTypeStoreSize(MemOpTy);
+        if (Info.isUniformStride() &&
----------------
I don't see anything here which requires the stride to be equal to the type size.  What prevents the set of pointers from e.g. advancing by 64 bytes where the access type is 8 bytes?  (i.e. what prevents this from being a strided access with non-unit stride in RISCV terms?)


================
Comment at: llvm/lib/Target/X86/X86TargetTransformInfo.cpp:4950
-  if (Info.isSameBase() && Info.isKnownStride()) {
-    // If all the pointers have known stride all the differences are translated
-    // into constants. X86 memory addressing allows encoding it into
----------------
KnownStride, and KnownUniform are not the same condition.  I don't think your code change in the general model actually matches what you removed here.

I'd suggest by starting with a RISCV specific hook on your heuristic, and then we can merge in a post commit.  I think the RISCV version is going to be more restrictive.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149654/new/

https://reviews.llvm.org/D149654