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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

In D74156#1869576 <https://reviews.llvm.org/D74156#1869576>, @lebedev.ri wrote:

> Let me know how to proceed.
>
> I know this doesn't scale, because i have designed it this way,
>  because i don't really expect it to be used for exhaustive sweeping,
>  but only exploring a few operands, because this is the real problem i have :]
>
> Is the traditional iterative approach not acceptable for this tool,
>  and the intention is to only get the the One True Solution,
>  even if it potentially takes ages before it's there?


I'm confused by your answer.

I am not against this patch, I just want to explain where I'd like the tool to go in the long run.
And since you responded to my comment with a code change and the creation of the `CombinationGenerator` I thought you were trying to implement what I suggested. Hence my answers.

If you don't intend to implement it, that's fine.

I've added a few 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);
----------------
formatting is weird here, maybe extract the lambda out of the constructor.


================
Comment at: llvm/unittests/tools/llvm-exegesis/SnippetGeneratorTest.cpp:37
+  };
+  ASSERT_EQ(Variants.size(), NumVariants);
+  ASSERT_EQ(Variants.size(), ExpectedVariants.size());
----------------
https://github.com/google/googletest/blob/master/googlemock/docs/cheat_sheet.md#container-matchers

You should be able to write
```
ASSERT_THAT(Variants, ElementsAreArray(ExpectedVariants));
```
or
```
ASSERT_THAT(Variants, ElementsAreArray({
      {0, 2},
      {0, 3},
      {1, 2},
      {1, 3},
  }));
```


================
Comment at: llvm/unittests/tools/llvm-exegesis/SnippetGeneratorTest.cpp:58
+  };
+  ASSERT_EQ(Variants.size(), NumVariants);
+  ASSERT_EQ(Variants.size(), ExpectedVariants.size());
----------------
ditto here and below


================
Comment at: llvm/unittests/tools/llvm-exegesis/SnippetGeneratorTest.cpp:186
+  ASSERT_EQ(Variants.size(), ExpectedVariants.size());
+  ASSERT_EQ(Variants, ExpectedVariants);
+}
----------------
```
ASSERT_THAT(Variants, IsEmpty());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74156