[PATCH] D102522: [llvm-exegesis] Loop unrolling for loop snippet repetitor mode

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

Cool, thanks for the change. I like the approach, only have minor comments.



================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:171
     // understands that the inside instructions are repeated.
-    constexpr const int kMinInstructionsForSnippet = 16;
+    const int kMinInstructionsForSnippet = 4 * Instructions.size();
+    const int kLoopBodySizeForSnippet = 2 * Instructions.size();
----------------
[style] This is no longer a constant, you can remove the `k`.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:172
+    const int kMinInstructionsForSnippet = 4 * Instructions.size();
+    const int kLoopBodySizeForSnippet = 2 * Instructions.size();
     {
----------------
ditto


================
Comment at: llvm/tools/llvm-exegesis/lib/SnippetRepetitor.h:43
+                              unsigned MinInstructions,
+                              unsigned LoopBodySize) const = 0;
 
----------------
Adding the `LoopBodySize` here sort of breaks the `SnippetRepetitor` abstraction. I think `LoopBodySize` should be a member of `LoopSnippetRepetitor`, initialized in the constructor. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102522