[PATCH] D71618: [System Model] Introduce system model classes
Michael Kruse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 17 13:30:17 PST 2019
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
More information about the llvm-commits
mailing list