Page MenuHomePhabricator

[System Model] Introduce system model classes
Needs ReviewPublic

Authored by greened on Dec 17 2019, 9:44 AM.

Details

Summary

Add classes to model system aspects such as caches and execution resources.
Eventually instances of these classes will be generated via TableGen and
MCSubtargetInfo instances will point to system model instances to satisfy
queries.

Diff Detail

Event Timeline

greened created this revision.Dec 17 2019, 9:44 AM
Meinersbur added inline comments.Dec 17 2019, 1:30 PM
llvm/include/llvm/MC/MCSystemModel.h
74

What are the IDs needed for? Isn't the object's pointer sufficient to identify it?

76

[typo] write-commbining

77

[style] I'd prefer const MCMemoryModel *MemModel = nullptr over initializing it to a constant in the dtor's initializer list.

292

[type] a core may have

405

[typo] describing

408–411

Doxygen comment must come before the the declaration they are describing or specially marked.

474

[typo] heirarchy

531

[nit] Doxygen comments come before the declaration

601–613

Does this describe the type CacheLevelList? Seems to belong somewhere else.

llvm/lib/MC/MCSystemModel.cpp
20–23

Why not define the dtors inline? Are the used to anchor the vtable?

llvm/unittests/MC/SystemModel.cpp
477–478

[style] use llvm::enumerate

537–552

[style] Why const for local variables?

fhahn added a subscriber: fhahn.Dec 18 2019, 1:09 AM

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

llvm/include/llvm/MC/MCSystemModel.h
74

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.

llvm/lib/MC/MCSystemModel.cpp
63

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?

79

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...

112

nit: unnecessary 'using'

llvm/unittests/MC/SystemModel.cpp
84

Threads per CPU or per core?

greened marked an inline comment as done.Feb 18 2020, 11:41 AM

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.

Currently our use is limited to very specific queries such as, "What is the size of the L2 cache?" or, "How many write-combining buffers are available?" When I first proposed the RFC it seemed like others thought interfaces for discovery (i.e. iteration) would be useful but I don't have a strong feeling about that.

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".

It should only be initialized once per model. Note that a target could have multiple models corresponding to different subtargets and maybe in SKUs within a subtarget (we currently have the former, per-subtarget models but not the latter, though as SKUs become much more specialized we're feeling pressure to add it).

Thanks for your feedback!

llvm/include/llvm/MC/MCSystemModel.h
74

Fine by me. I was just following the example of other pieces of the MC layer.

Matt added a subscriber: Matt.Apr 20 2021, 8:23 AM