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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:788
+                                decltype(VariableChoices)::value_type, 4>
+  G(VariableChoices, [&](ArrayRef<MCOperand> State) -> bool {
+    Variants.emplace_back(&Instr);
----------------
gchatelet wrote:
> formatting is weird here, maybe extract the lambda out of the constructor.
Hm, rerun clang-format on this, and this time it produced different result. Is this better?


================
Comment at: llvm/unittests/tools/llvm-exegesis/SnippetGeneratorTest.cpp:186
+  ASSERT_EQ(Variants.size(), ExpectedVariants.size());
+  ASSERT_EQ(Variants, ExpectedVariants);
+}
----------------
gchatelet wrote:
> ```
> ASSERT_THAT(Variants, IsEmpty());
> ```
But it's not empty, it contains a single element `0`?


================
Comment at: llvm/unittests/tools/llvm-exegesis/SnippetGeneratorTest.cpp:178
+  ASSERT_THAT(Variants, ::testing::SizeIs(NumVariants));
+  ASSERT_THAT(Variants, ::testing::ContainerEq(ExpectedVariants));
+}
----------------
gchatelet wrote:
> It's not really empty then. maybe rename with `Singleton`
By empty i meant that the "exploration" space is empty - there is only a single possibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74156