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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Hi David,

Glad to see this taking shape. Thanks for working on this!

So far, it's mostly mechanical and gives some idea of the hierarchy, but until we get to use it in real code, it'll be hard to know what's the most efficient model. It will depend on which are the most likely queries, if they're mostly independent or iterable, etc. If you have any insight on the matter, it would be good to share here, so it's easier to review.

The init methods are quite heavy but I'm assuming they only run once per target init. They do run list.clear() before, which may mean there's later re-initialisation, or maybe it's just for "extra safety".

Some comments inline, but overall, looking nice.

Thanks!
--renato



================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:74
+private:
+  unsigned ID;                    /// A unique ID
+  const char *Name;               /// A name for debugging
----------------
Meinersbur wrote:
> What are the IDs needed for? Isn't the object's pointer sufficient to identify it?
Yeah, I also don't see the point of the ID. If there was an enum per target or something to identify, ok, but having loose IDs seems more complicated than indexing via pointer.


================
Comment at: llvm/lib/MC/MCSystemModel.cpp:63
+  //          L1 (C) L1 (L) L1(B)
+  //
+  // The algorithm below recursively determines the topology for the
----------------
So, imagining two sockets with 48 cores + L3, and 4 clusters of 12 cores + L2 in each socket, and each core with L1, this would be:

    L3 (CPU0) L3 (CPU1)
    L2 (Cluster0_0) L2 (Cluster0_1) ... L2 (Cluster1_3)
    L1 (Core0) L1 (Core1) ... L1 (Core96)

or

    L3 (CPUs)
    L2 (Clusters)
    L1 (Cores)

and the hiearchy would realise which socket a cluster belongs to and which cluster a core belongs to?



================
Comment at: llvm/lib/MC/MCSystemModel.cpp:79
+      if (i < I2.size())
+        Result.back().append(I2[i].begin(), I2[i].end());
+    }
----------------
CacheLevelInfo is initialised with 4 elements (SmallVector<4>), and this looks like it will grow considerably and quite likely reallocate a lot. Maybe you can guess the final size and resize() it before the loop?

Also, SmallVector probably won't be better than std:vector here...


================
Comment at: llvm/lib/MC/MCSystemModel.cpp:112
+
+  using WorkListType = SmallVector<const MCExecutionResourceDesc *, 4>;
+  WorkListType WorkList;
----------------
nit: unnecessary 'using'


================
Comment at: llvm/unittests/MC/SystemModel.cpp:84
+  unsigned CPUCoreCounts[]   = { 2,         8 };
+  unsigned CPUThreadCounts[] = { 4,         2 };
+
----------------
Threads per CPU or per core?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71618