[PATCH] D44488: [llvm-mca] Refactor class RegisterFile to allow the definition of multiple register files.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few minor comments but otherwise looks almost ready.



================
Comment at: tools/llvm-mca/Dispatch.cpp:154
+  SmallVector<unsigned, 4> NumTemporaries(0U);
+  NumTemporaries.resize(getNumRegisterFiles());
+
----------------
Better to do this in the constructor?


================
Comment at: tools/llvm-mca/Dispatch.h:31
 
-/// \brief Keeps track of register definitions.
-///
-/// This class tracks register definitions, and performs register renaming
-/// to break anti dependencies.
-/// By default, there is no limit in the number of register aliases which
-/// can be created for the purpose of register renaming. However, users can
-/// specify at object construction time a limit in the number of temporary
-/// registers which can be used by the register renaming logic.
+/// \brief Manages hardware register files, and and tracks data dependencies
+/// between registers.
----------------
and and


================
Comment at: tools/llvm-mca/Dispatch.h:78
+  // Note that this implementation allows register files to overlap.
+  using RegisterMapping = std::pair<WriteState *, unsigned>;
+
----------------
Assert that there 32 or less PRFs?


================
Comment at: tools/llvm-mca/Dispatch.h:80
+
+  // This map contains one entry for ech physical register defined by the
+  // processor scheduling model.
----------------
for each


================
Comment at: tools/llvm-mca/Dispatch.h:84
+
+  // Scheduling models can optionally describe register files.
+  // For example, here is how a tablegen definitions for a x86 FP register file
----------------
Please can you make it clear in the comment this WIP?


================
Comment at: tools/llvm-mca/Dispatch.h:91
+  // Here FPRegisterFile contains all the registers defined by register class
+  // VR128RegClass and VR26RegClass. FPRegisterFile implements 60
+  // registers which can be used for register renaming purpose.
----------------
VR256RegClass


================
Comment at: tools/llvm-mca/Dispatch.h:140
+  unsigned getMaxUsedRegisterMappings(unsigned RegisterFileIndex) const {
+    assert(RegisterFileIndex < getNumRegisterFiles());
+    return RegisterFiles[RegisterFileIndex].MaxUsedMappings;
----------------
(style) Asserts should have a message as well as a condition.


https://reviews.llvm.org/D44488