[PATCH] D52041: [llvm-exegesis] Allow benchmarking arbitrary code snippets.

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:39
   std::vector<llvm::MCInst> Code = BC.Instructions;
-  for (int I = 0; I < MinInstructions; ++I)
+  for (int I = BC.Instructions.size(); I < MinInstructions; ++I)
     Code.push_back(BC.Instructions[I % BC.Instructions.size()]);
----------------
```while(Code.size() < (BC.size() + MinInstruction))```


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:108
+static unsigned getOpcodeOrDie(const llvm::MCInstrInfo &MCInstrInfo) {
+  if ((!OpcodeName.empty() + !(OpcodeIndex == 0) + !SnippetsFile.empty()) != 1)
     llvm::report_fatal_error(
----------------
Can we make this more explicit?
```
const size_t CountSetFlags =
 (OpcodeName.empty() ? 0 : 1) +
 (OpcodeIndex == 0 ? 0 : 1) +
 (SnippetsFile.empty() ? 0 : 1);
if(CountSetFlags != 1)
   ...
```


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:159
 }
+namespace {
+
----------------
newline before


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:161
+
+// A streamer that reads a BenchmarkCode definition from a file.
+// The BenchmarkCode definition is just an asm file, which additional comments
----------------
`A streamer ` -> `An MCStreamer `


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:162
+// A streamer that reads a BenchmarkCode definition from a file.
+// The BenchmarkCode definition is just an asm file, which additional comments
+// to specify which registers should be defined or are live on entry.
----------------
`which` -> `with`


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:213
+  unsigned findRegisterByName(const llvm::StringRef RegName) const {
+    // FIXME: Can we do better than this ?
+    for (unsigned I = 0, E = RegInfo->getNumRegs(); I < E; ++I) {
----------------
AFAIK llvm does not provide any tool to do it. Not sure if it's worth the addition though.


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:231
+static llvm::Expected<std::vector<BenchmarkCode>>
+readSnippets(const LLVMState &State, std::string Filename) {
+  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> BufferPtr =
----------------
`std::string Filename` -> `llvm::StringRef Filename` ?


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:236
+    llvm::errs() << Filename << ": " << EC.message() << '\n';
+    return llvm::make_error<BenchmarkFailure>("cannot read snippet");
+  }
----------------
Why pass the filename/message in the BenchmarkFailure and remove the llvm::errs line?


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:254
+    return llvm::make_error<BenchmarkFailure>("cannot create asm parser");
+  AsmParser->setAssemblerDialect(0); // FIXME: allow changing the dialect.
+  AsmParser->getLexer().setCommentConsumer(&Streamer);
----------------
Add a comment explaining what 0 is in this context.


Repository:
  rL LLVM

https://reviews.llvm.org/D52041