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

Tue Feb 18 11:48:33 PST 2020

greened marked an inline comment as done.
greened added a comment.

In D71618#1789459 <https://reviews.llvm.org/D71618#1789459>, @rengolin wrote:

>

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

================
Comment at: llvm/include/llvm/MC/MCSystemModel.h:74
+private:
+  unsigned ID;                    /// A unique ID
+  const char *Name;               /// A name for debugging
----------------
rengolin wrote:
> 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.
Fine by me.  I was just following the example of other pieces of the MC layer.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71618