[PATCH] D46821: Update llvm-exegesis to cover latency through another opcode.

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: tools/llvm-exegesis/lib/AliasingTracker.h:41
+// Tracker.getOrigin(llvm::X86::BX) == -1;
+struct AliasingTracker {
+  // Construct a tracker from an MCRegisterClass.
----------------
`RegisterAliasingTracker` because `Aliasing` has too many meanings.


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.cpp:152
+
+std::mt19937 &RandomGenerator() {
+  static std::random_device RandomDevice;
----------------
[style]randomGenerator


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.cpp:154
+  static std::random_device RandomDevice;
+  static std::mt19937 RandomGenerator(RandomDevice());
+  return RandomGenerator;
----------------
let's seed with a flag or a constexpr (unittests will thank you).


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.cpp:160
+
+size_t RandomIndex(size_t Size) {
+  assert(Size > 0);
----------------
[style] static


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.cpp:160
+
+size_t RandomIndex(size_t Size) {
+  assert(Size > 0);
----------------
courbet wrote:
> [style] static
[style]randomIndex


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.cpp:162
+  assert(Size > 0);
+  std::uniform_int_distribution<> dis(0, Size - 1);
+  return dis(RandomGenerator());
----------------
[style]Dis



================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.cpp:167
+template <typename C>
+auto RandomElement(const C &Container) -> decltype(Container[0]) {
+  return Container[RandomIndex(Container.size())];
----------------
[style]  static+randomElement


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.cpp:179
+  case llvm::MCOI::OperandType::OPERAND_IMMEDIATE:
+    Variable.AssignedValue = llvm::MCOperand::createImm(1);
+    break;
----------------
Add a FIXME to explore immediate values too.


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.cpp:205
+
+size_t RandomBit(const llvm::BitVector &Vector) {
+  assert(Vector.any());
----------------
[style] randomBit


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.h:79
+// Represents the assignment of a Register to an Operand.
+struct RegisterOperandValue {
+  RegisterOperandValue(const Operand *Operand, llvm::MCPhysReg Value)
----------------
`RegisterOperandAssignment` ?


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.h:84
+  const Operand *Operand; // Pointer to an Explicit Register Operand.
+  llvm::MCPhysReg Value;
+
----------------
Reg


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.h:91
+// Represents a set of Operands that would alias through the use of some
+// Registers.
+struct AliasingRegisterOperands {
----------------

```
// There are two reasons why operands would alias:
// - The registers assigned to each of the operands are the same or alias each other (e.g. AX/AL)
// - The operands are tied.
```


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.h:115
+  const Instruction &UseInstruction;
+  SmallSet<AliasingRegisterOperands, 32> Configurations;
+};
----------------
Let's get rid of SmallSet and use std::find


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.h:119
+// A global Random Number Generator to randomize configurations.
+std::mt19937 &RandomGenerator();
+
----------------
ideally this would come from the main (or tests). Possibly in a subsequent revision.


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.h:127
+// it and set the target Variables to the selected values.
+void setRandomAliasing(const AliasingConfigurations &AliasingConfigurations);
+
----------------
```
AliasingConfigurations AliasingConfigurations::createRandomAssignment(const AliasingConfigurations &AliasingConfigurations);
```



================
Comment at: tools/llvm-exegesis/lib/PerfHelper.cpp:113
+  if (ReadSize != sizeof(Count)) {
+    Count = -1;
     llvm::errs() << "Failed to read event counter\n";
----------------
I submitted this independently for you as r332330, please rebase.


================
Comment at: tools/llvm-exegesis/lib/Uops.cpp:83
 
-// FIXME: Handle memory (see PR36906)
-static bool isInvalidOperand(const llvm::MCOperandInfo &OpInfo) {
-  switch (OpInfo.OperandType) {
-  default:
-    return true;
-  case llvm::MCOI::OPERAND_IMMEDIATE:
-  case llvm::MCOI::OPERAND_REGISTER:
-    return false;
-  }
+namespace {
+
----------------
static


Repository:
  rL LLVM

https://reviews.llvm.org/D46821