[PATCH] D74156: [llvm-exegesis] Exploring X86::OperandType::OPERAND_COND_CODE

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

First, thank you for the patch, this is going in the right direction.

Now stepping back a bit there are many dimensions that we'd like to explore:

- argument values (which is what you started here),
- register selection (registers of a class are not strictly equivalent [1])
- snippet generation (we always select the same pattern but exploring them would help [2])
- repetition mode (to see the impact of the decoder)

Code wise this means it would be much better to have a global sampler object responsible for how to explore these dimensions rather than the greedy approach we're heading to.
Now I understand this is a substantial redesign and I'm not asking you to do it but I just wanted to share what I believe is the right direction to lower code complexity in the long run.

---

[1] `LEA` is known to produce different latencies <https://github.com/golang/go/issues/21735> when using EBP, RBP, or R13 <https://reviews.llvm.org/source/pstl/> as base registers
[2] for instance

  XOR EAX, EAX, EAX

is self dependent but it's also a zero idiom, if we'd also executed the back to back pattern we would have learned something new

  XOR EBX, EAX, EAX
  XOR EAX, EBX, EBX



================
Comment at: llvm/tools/llvm-exegesis/lib/SnippetGenerator.cpp:91
+          // We exceeded the number of  allowed configs.
+          // This isn't really an error, but early out regardless.
+          return Error::success();
----------------
I don't think it's worth mentioning error here.
Maybe replace the whole comment with
```
We reached the number of  allowed configs and return early.
```


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:749
 
+static bool RecursiveCombinationGenerator(
+    ArrayRef<SmallVector<MCOperand, 1>> VariableChoices,
----------------
This function deserves some documentation


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:775
+  // different operands affect performance. So for each operand position,
+  // precompute all the possible choices we might care about.
+  SmallVector<SmallVector<MCOperand, 1>, 4> VariableChoices;
----------------
This comment should be at the function level (possibly at the function declaration)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74156