[PATCH] D71618: [System Model] Introduce system model classes

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:74
+private:
+  unsigned ID;                    /// A unique ID
+  const char *Name;               /// A name for debugging
----------------
What are the IDs needed for? Isn't the object's pointer sufficient to identify it?


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:76
+  const char *Name;               /// A name for debugging
+  const int NumBuffers;           /// The number of write-commbining buffers
+  const MCMemoryModel *MemModel;  /// The associated memory model
----------------
[typo] write-com**m**bining


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:77
+  const int NumBuffers;           /// The number of write-commbining buffers
+  const MCMemoryModel *MemModel;  /// The associated memory model
+
----------------
[style] I'd prefer `const MCMemoryModel *MemModel = nullptr` over initializing it to a constant in the dtor's initializer list.


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:292
+/// model of the memory system as viewed from a particular execution resource.
+/// For example, a core my have L1 and L2 caches private to it, while a socket
+/// may have an L3 shared by all cores contained by the socket.  The core memory
----------------
[type] a core m**a**y have


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:405
+/// entirely equivalent objects to describe the threads.  Instead we instantiate
+/// one MCExecutionResourceDesc desribing 48 cores containing one
+/// MCExecutionResourceDesc describing four threads.
----------------
[typo] des**c**ribing 


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:408-411
+  unsigned ID;                          /// A unique ID
+  const char *Name;                     /// A name for debugging
+  const MCExecutionResource *Resource;  /// The described resource
+  unsigned NumResources;                /// The resource count
----------------
Doxygen comment must come before the the declaration they are describing or [[ http://doxygen.nl/manual/docblocks.html#memberdoc | specially marked ]].


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:474
+/// thread, etc.).  A resource contains a memory model describing the pieces of
+/// the memory heirarchy private to it.  For example a core may contain two
+/// levels of private cache and the socket containing the cores may contain one
----------------
[typo] h**ei**rarchy 


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:531
+
+  virtual ~MCExecutionResource();     /// Allow subclasses
+
----------------
[nit] Doxygen comments come before the declaration


================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:601-613
+  /// Make the cache topology indexable by level.  The bottom-most
+  /// cache level of each resource makes up level zero.  For example:
+  //
+  ///  Thread
+  ///     |
+  ///  Big Core  Little Core
+  ///          \/
----------------
Does this describe the type `CacheLevelList`? Seems to belong somewhere else.


================
Comment at: llvm/lib/MC/MCSystemModel.cpp:20-23
+MCWriteCombiningBufferInfo::~MCWriteCombiningBufferInfo() {}
+MCSoftwarePrefetcherConfig::~MCSoftwarePrefetcherConfig() {}
+MCCacheLevelInfo::~MCCacheLevelInfo() {}
+MCMemoryModel::~MCMemoryModel() {}
----------------
Why not define the dtors inline? Are the used to anchor the vtable?


================
Comment at: llvm/unittests/MC/SystemModel.cpp:477-478
+
+  i = 0;
+  for (const auto L2Level : L2Levels) {
+    const char *const *CacheNames        = (i == 0 ? BigCacheLevelNames :
----------------
[style] use `llvm::enumerate`


================
Comment at: llvm/unittests/MC/SystemModel.cpp:537-552
+  const unsigned BigL1 = 0;
+  const unsigned BigL2 = 1;
+
+  const unsigned LittleL1 = 0;
+
+  const unsigned GPUCoreL1 = 0;
+
----------------
[style] Why const for local variables?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71618