[PATCH] D68125: [llvm-exegesis] Add loop mode for repeating the snippet.

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/Assembler.cpp:122
+                                       const llvm::DebugLoc &DL) {
+  for (const MCInst &Inst : Insts) {
+    addInstruction(Inst, DL);
----------------
remove braces


================
Comment at: llvm/tools/llvm-exegesis/lib/Assembler.cpp:219
 
+  Fill(Filler);
+
----------------
How about `FillWith`?


================
Comment at: llvm/tools/llvm-exegesis/lib/SnippetRepetitor.cpp:67
+           ET.setRegTo(State.getSubtargetInfo(), LoopCounter,
+                       APInt(32, (MinInstructions + Instructions.size() - 1) /
+                                     Instructions.size()))) {
----------------
Can you extract the constant to a varaible?


================
Comment at: llvm/tools/llvm-exegesis/lib/SnippetRepetitor.cpp:78
+      Loop.MBB->addLiveIn(LoopCounter);
+      for (unsigned Reg : Filler.getRegistersSetUp()) {
+        Loop.MBB->addLiveIn(Reg);
----------------
Remove braces


================
Comment at: llvm/tools/llvm-exegesis/lib/SnippetRepetitor.cpp:81
+      }
+      for (const auto& LiveIn : Entry.MBB->liveins()) {
+        Loop.MBB->addLiveIn(LiveIn);
----------------
ditto


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:507
+  }
+  return llvm::X86::R8;
+}
----------------
Add a comment on why `R8`


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:557
+  BuildMI(&MBB, DebugLoc(), MII.get(X86::ADD64ri8))
+      .addDef(X86::R8)
+      .addUse(X86::R8)
----------------
Maybe use a constant instead of repeating `R8` everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68125