[PATCH] D58736: [System Model] Introduce a target system model

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thank you for pushing this forward and sorry for the delay.

Could you add some central high-level documentation about what the memory system model is? E.g. describe that an `MCSystemModel` has a list of execution resources, memory hierarchies, prefetch configs and write-combining buffers. A Cache hierarchy as a total size, line size, associativity, etc. To get the interpretation eight, please add more details about ever parameter, particularly the prefetch configs. Some other examples than ARM big.LITTLE would be nice as well.

What exactly is a "prefetch config"? Is there a prefetch config for each cache level? Different hardware mechanisms for prefetch (e.g. stride detection or software-estabslished). Different strategies for inserting prefetch instruction selectable at compile-time?

AFAICS, this patch does uses the default model for all targets?



================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:735-742
+  /// \return Whether to prefetch loads.
+  bool prefetchReads() const;
+
+  /// \return Whether to prefetch stores.
+  bool prefetchWrites() const;
+
+  /// \return Whether to use read prefetches for stores.
----------------
[bikeshedding] The name should indicate that these methods just return some information. How about `isPrefetchingReads`/etc.?


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1136
       const Instruction &I, bool &AllowPromotionWithoutCommonHeader) = 0;
-  virtual unsigned getCacheLineSize() = 0;
-  virtual llvm::Optional<unsigned> getCacheSize(CacheLevel Level) = 0;
-  virtual llvm::Optional<unsigned> getCacheAssociativity(CacheLevel Level) = 0;
-  virtual unsigned getPrefetchDistance() = 0;
-  virtual unsigned getMinPrefetchStride() = 0;
-  virtual unsigned getMaxPrefetchIterationsAhead() = 0;
+  virtual unsigned getCacheLineSize() const = 0;
+  virtual llvm::Optional<unsigned> getCacheSize(CacheLevel Level) const = 0;
----------------
It is interesting that getCacheSize/getCacheAssociativity return `llvm::Optional`s and are cache-level specific, but not getCacheLineSize


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfoImpl.h:403-407
+  unsigned getPrefetchDistance() const { return 0; }
+
+  unsigned getMinPrefetchStride() const { return 1; }
+
+  unsigned getMaxPrefetchIterationsAhead() const { return UINT_MAX; }
----------------
Could you add some documentation about what these special values mean?


================
Comment at: llvm/include/llvm/MC/MCSubtargetInfo.h:58-65
+  /// If caches at a particular level are of different sizes, ask the
+  /// target what to do.  By default, return zero.
+  ///
+  virtual Optional<unsigned>
+  resolveCacheSize(unsigned Level,
+                   const MCSystemModel::CacheLevelSet &Levels) const {
+    return Optional<unsigned>(0);
----------------
I don't understand the idea of the `resolve...` API. What is a target overriding it supposed to do? There are not overrides in this patch. Why not providing a default implementation of `getCacheSize` that targets can override?


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:406-407
+  /// within this one.
+  ///
+  ///
+  unsigned getNumContainedExecutionResourceTypes() const {
----------------
[style] These trailing empty comment lines don't seem to be useful. The current LLVM code base does not have them.


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:443
+
+  static const MCSystemModel Default;
+
----------------
Could we avoid the static initializer?


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:464
+
+  using CacheLevelSet = SmallVector<const MCCacheLevelInfo *, 4>;
+
----------------
The type has 'Set' in its name, but is an alias for a vector?


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:468
+  using CacheLevelInfo = SmallVector<CacheLevelSet, 4>;
+  mutable CacheLevelInfo CacheLevels;
+
----------------
Why does it need to be `mutable`?


================
Comment at: llvm/include/llvm/Target/TargetSoftwarePrefetchConfig.td:44-47
+  int InstructionsAhead = id.Value;    // Prefetch this many instructions ahead,
+                                       // used by prefetchers that operate in
+                                       // terms of instruction distance rather
+                                       // than bytes.
----------------
ISA instructions or µOps? Why not cycles?


================
Comment at: llvm/include/llvm/Target/TargetSoftwarePrefetchConfig.td:48-52
+  int MaxIterationsAhead = ixd.Value;  // Don't prefetch more than this number
+                                       // of iterations ahead.  Used by
+                                       // prefetchers that operate in terms of
+                                       // instruction distance rather than
+                                       // bytes.
----------------
Isn't this algorithm-dependent, i.e. the size of the loop?


================
Comment at: llvm/lib/MC/MCSubtargetInfo.cpp:144
+  if (Found == Models.end() || StringRef(Found->Key) != CPU) {
+    if (CPU != "help") // Don't error if the user asked for help.
+      errs() << "'" << CPU
----------------
I find this handling of `--help` strange, but the current `MCSubtargetInfo::getSchedModelForCPU` does the same thing.


================
Comment at: llvm/lib/MC/MCSystemModel.cpp:27-48
+  MCWriteCombiningBufferInfo DefaultWCBuffers(0,
+                                              "Default Write-Combining Buffer",
+                                              0);
+
+  MCSoftwarePrefetcherConfig DefaultPrefetcherConfig(0,
+                                                     "Default Prefetcher",
+                                                     false,
----------------
where are these used?


================
Comment at: llvm/lib/MC/MCSystemModel.cpp:66
+  // we have a system with a CPU socket and a GPU socket.  The CPU
+  // socket contains two core types: big and lttle.  The CPUsocket
+  // contains an L3 cache, the big core contains and L2 and L1 cache
----------------
[typo] `lttle`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58736