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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:884
+  /// \param NumStridedMemAccesses Number of strided memory accesses in the loop.
+  /// \param NumPrefetches Number of prefetches needed for the strided accesses.
+  /// \param HasCall True if the loop contains a call.
----------------
I am confused with the description of `NumPrefetches`. It looks like its passed the number of different memory accesses in the loop. "Number of prefetches needed for the strided accesses." sounds like hardware requirement, but its passed a property of the code. Can you clarify?


================
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:
> Meinersbur wrote:
> > 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.
> I don't think that's possible given that it's also used by the ORE, or?
> 
Well, then it's not only for debug output as the comment says. Remove the part in parens?

I'd appreciate if the other class members had doxygen comments as well.


================
Comment at: llvm/lib/Transforms/Scalar/LoopDataPrefetch.cpp:237
+
+  Prefetch(const SCEVAddRecExpr *lscev, Instruction *i)
+      : LSCEVAddRec(lscev), InsertPt(nullptr), Writes(false), MemI(nullptr) {
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | Start parameter names with capital letters (PascalCase) ]]


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

https://reviews.llvm.org/D70228