[PATCH] D48713: [llvm-exegesis] Add uop computation for more X87 instruction classes.

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:224
+  SnippetPrototype Prototype;
+  Prototype.Explanation = Msg.str();
+  Prototype.Explanation += ", repeating an unconstrained assignment";
----------------
llvm::formatv("{0}, repeating an unconstrained assignment", Msg);


================
Comment at: tools/llvm-exegesis/lib/X86/Target.cpp:141
+    if (Reg == llvm::X86::EFLAGS) {
+      constexpr const uint32_t kImmValue = 0x00007002u;
+      std::vector<llvm::MCInst> Result;
----------------
Explain the magic value.


================
Comment at: tools/llvm-exegesis/lib/X86/Target.cpp:148
+    }
+    llvm::errs() << Reg << "\n";
     return {};
----------------
?


================
Comment at: tools/llvm-exegesis/lib/X86/Target.cpp:171
   static std::vector<llvm::MCInst>
   setVectorRegToConstant(const unsigned Reg, const unsigned RegSizeBytes,
                          const unsigned RMOpcode) {
----------------
Reg and RegSizeBytes could theoretically be out of sync. Should RegSizeBytes be computed from Reg?


================
Comment at: tools/llvm-exegesis/lib/X86/Target.cpp:182
+    Result.push_back(allocateStackSpace(RegSizeBytes));
     for (unsigned Disp = 0; Disp < RegSizeBytes; Disp += 4) {
+      Result.push_back(fillScratchMemory(llvm::X86::MOV32mi, Disp, kImmValue));
----------------
Maybe create a constant for 4 to give it semantic.


================
Comment at: tools/llvm-exegesis/lib/X86/Target.cpp:199
+  // Fills scratch memory at offset `OffsetBytes` with value `Imm`.
+  static llvm::MCInst fillScratchMemory(unsigned MovOpcode,
+                                        unsigned OffsetBytes, uint64_t Imm) {
----------------
`fillStackSpace`? `fillScratchMemory` feels too broad and disconnected from `allocateStackSpace`


Repository:
  rL LLVM

https://reviews.llvm.org/D48713