[PATCH] D71653: [llvm-exegesis][NFC] internal changes

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/CodeTemplate.cpp:18
 
 InstructionTemplate::InstructionTemplate(const Instruction &Instr)
+    : Instr(&Instr), VariableValues(Instr.Variables.size()) {}
----------------
Let's take `Instr` by pointer to avoid potentially binding to temporaries.


================
Comment at: llvm/tools/llvm-exegesis/lib/MCInstrDescView.cpp:86
+      return Entry.get();
+  auto &Entry = Cache.emplace_back(new BitVector());
+  Entry->swap(BV);
----------------
`push_back(make_unique)` ? I don't think there's a good reason to `new` here.


================
Comment at: llvm/tools/llvm-exegesis/lib/MCInstrDescView.h:96
+/// For X86, this is ~160 unique vectors for all of the ~15K Instructions.
+struct UniqueBitVector {
+  const BitVector *get(BitVector &&) const;
----------------
The name is a bit misleading. To me, an `XXXBitVector` is a `BitVector`. `BitVectorCache` would be better I think.


================
Comment at: llvm/tools/llvm-exegesis/lib/MCInstrDescView.h:97
+struct UniqueBitVector {
+  const BitVector *get(BitVector &&) const;
+
----------------
Doc ? This is not obvious.


================
Comment at: llvm/tools/llvm-exegesis/lib/MCInstrDescView.h:100
+private:
+  mutable SmallVector<std::unique_ptr<BitVector>, 0> Cache;
 };
----------------
`SmallVector<T, 0>` ? Why not `std::vector` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/MCInstrDescView.h:106
 struct Instruction {
-  Instruction(const MCInstrInfo &InstrInfo,
-              const RegisterAliasingTrackerCache &RATC, unsigned Opcode);
+  Instruction(const MCInstrDesc *Description, StringRef Name,
+              SmallVector<Operand, 8> Operands,
----------------
can this be private ?


================
Comment at: llvm/tools/llvm-exegesis/lib/MCInstrDescView.h:119
+  static std::unique_ptr<Instruction>
+  create(const MCInstrInfo &InstrInfo, const RegisterAliasingTrackerCache &RATC,
+         const UniqueBitVector &UBV, unsigned Opcode);
----------------
Move this to the top ? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71653