[PATCH] D70228: [LoopDataPrefetch + SystemZ] Let target decide on prefetching on a per loop basis

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

@greened is looking into generating the prefetcher properties using tblgen (D58736 <https://reviews.llvm.org/D58736>). Would this be compatible with that approach?

I like the clean test cases. Unfortunely, I don't have experience with prefetching in the lbm benchmark.



================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1324-1327
+  virtual unsigned getMinPrefetchStride(unsigned NumMemAccesses,
+                                        unsigned NumStridedMemAccesses,
+                                        unsigned NumPrefetches,
+                                        bool HasCall) const = 0;
----------------
Could you add explanations for the individual parameters to the doxygen comment?


================
Comment at: llvm/lib/Transforms/Scalar/LoopDataPrefetch.cpp:232
+  /// The prefetched instruction (for debug output)
+  Instruction* MemI;
+  Prefetch(const SCEVAddRecExpr *lscev, Instruction *i) :
----------------
jonpa wrote:
> The Prefetch struct MemI member is used solely for debug/ORE output. It is currently not guarded by '#ifndef NDEBUG', but maybe it should be, even though it's just one pointer? The debug output could perhaps be improved also, I merely tried to keep what there was while also printing the new values gathered. The MemI member is not needed for anything else, so if we could change the debug output instead perhaps it's not even needed, or?
I'd appreciate if it was guarded by `#ifndef NDEBUG`. It would make it more explicit and enforcing to be only used in debug builds.


================
Comment at: llvm/lib/Transforms/Scalar/LoopDataPrefetch.cpp:238
+  };
+  void addInstruction(Instruction *I, DominatorTree *DT = nullptr,
+                      int64_t PtrDiff = 0) {
----------------
[style] Please add empty lines between methods.

Also, add a doxygen comment about what this method and its parameters (and the class itself) are supposed to do.


================
Comment at: llvm/lib/Transforms/Scalar/LoopDataPrefetch.cpp:244-245
+      Writes = isa<StoreInst>(I);
+    }
+    else {
+      BasicBlock *PrefBB = InsertPt->getParent();
----------------
[style] please clang-format the patch.


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

https://reviews.llvm.org/D70228