[exegesis][X86] ParallelSnippetGenerator: don't accidentally create serialized instructions

[lebedev.ri]

Summary

In the case of no tied variables, we pick random defs, and then random uses that don't alias with defs we just picked.
Sounds good, except that an X86 instruction may have implicit reg uses,
e.g. for MULX it's EDX/RDX: Intel SDM, 4-162 Vol. 2B MULX — Unsigned Multiply Without Affecting Flags

Performs an unsigned multiplication of the implicit source operand (EDX/RDX) and the specified source operand
(the third operand) and stores the low half of the result in the second destination (second operand), the high half
of the result in the first destination operand (first operand), without reading or writing the arithmetic flags.

And indeed, every once in a while llvm-exegesis happened to pick EDX as a def while measuring throughput,
and producing garbage output:

$ ./bin/llvm-exegesis -num-repetitions=1000000 -mode=inverse_throughput -repetition-mode=min --loop-body-size=4096 -dump-object-to-disk=false -opcode-name=MULX32rr --max-configs-per-opcode=65536
---
mode:            inverse_throughput
key:
  instructions:
    - 'MULX32rr EDX R11D R12D'
  config:          ''
  register_initial_values:
    - 'R12D=0x0'
    - 'EDX=0x0'
cpu_name:        znver3
llvm_triple:     x86_64-unknown-linux-gnu
num_repetitions: 1000000
measurements:
  - { key: inverse_throughput, value: 4.00014, per_snippet_value: 4.00014 }
error:           ''
info:            instruction has no tied variables picking Uses different from defs
assembled_snippet: 415441BC00000000BA00000000C4C223F6D4C4C223F6D4C4C223F6D4C4C223F6D4415CC3415441BC00000000BA0000000049B80200000000000000C4C223F6D4C4C223F6D44983C0FF75F0415CC3
...

$ ./bin/llvm-exegesis -num-repetitions=1000000 -mode=inverse_throughput -repetition-mode=min --loop-body-size=4096 -dump-object-to-disk=false -opcode-name=MULX32rr --max-configs-per-opcode=65536
---
mode:            inverse_throughput
key:
  instructions:
    - 'MULX32rr R13D EDX ECX'
  config:          ''
  register_initial_values:
    - 'ECX=0x0'
    - 'EDX=0x0'
cpu_name:        znver3
llvm_triple:     x86_64-unknown-linux-gnu
num_repetitions: 1000000
measurements:
  - { key: inverse_throughput, value: 3.00013, per_snippet_value: 3.00013 }
error:           ''
info:            instruction has no tied variables picking Uses different from defs
assembled_snippet: 4155B900000000BA00000000C4626BF6E9C4626BF6E9C4626BF6E9C4626BF6E9415DC34155B900000000BA0000000049B80200000000000000C4626BF6E9C4626BF6E94983C0FF75F0415DC3
...

Oops! Not only does that not look fun, i did hit that pitfail during AMD Zen 3 enablement.
While i have since then addressed this in rGd4d459e7475b4bb0d15280f12ed669342fa5edcd,
i suspect there may be other buggy results lying around, so we should at least stop producing them.

[lebedev.ri]

ping @courbet @gchatelet

[courbet]

Thanks for the patch. Give me some time to add a unit test that this would break, I'll ping here when submitted.

[courbet]

Done, please rebase to rG131f7bac63b8 and you should now have a failing test.

[lebedev.ri]

Forgot to say, thank you for the review!

llvm/tools/llvm-exegesis/lib/ParallelSnippetGenerator.cpp

ParallelSnippetGenerator::generateCodeTemplates(
   if (!TiedVariables.empty()) {
     CT.Info = "instruction has tied variables, using static renaming.";
     CT.Instructions = generateSnippetUsingStaticRenaming(
         State, Variant, TiedVariables, ForbiddenRegisters);
     instantiateMemoryOperands(CT.ScratchSpacePointerInReg, CT.Instructions);
     return getSingleton(std::move(CT));
   }
   // No tied variables, we pick random values for defs.
+
+  // We don't want to accidentally serialize the instruction,
+  // so we must be sure that we don't pick a def that is an implicit use,
+  // or a use that is an implicit def, so record implicit regs now.
+  BitVector ImplicitUses(State.getRegInfo().getNumRegs());
+  BitVector ImplicitDefs(State.getRegInfo().getNumRegs());
+  for (const auto &Op : Instr.Operands) {
+    if (Op.isReg() && Op.isImplicit() && !Op.isMemory()) {
+      assert(Op.isImplicitReg() && "Not an implicit register operand?");
+      if (Op.isUse())
+        ImplicitUses.set(Op.getImplicitReg());
+      else {
+        assert(Op.isDef() && "Not a use and not a def?");
+        ImplicitDefs.set(Op.getImplicitReg());
+      }
+    }
+  }
+  const auto ImplicitUseAliases =
+      getAliasedBits(State.getRegInfo(), ImplicitUses);
+  const auto ImplicitDefAliases =
+      getAliasedBits(State.getRegInfo(), ImplicitDefs);
   BitVector Defs(State.getRegInfo().getNumRegs());
   for (const auto &Op : Instr.Operands) {
     if (Op.isReg() && Op.isExplicit() && Op.isDef() && !Op.isMemory()) {
       auto PossibleRegisters = Op.getRegisterAliasing().sourceBits();
-      // Do not use forbidden registers.
+      // Do not use forbidden registers and regs that are implicitly used.
+      // Note that we don't try to avoid using implicit defs explicitly.
       remove(PossibleRegisters, ForbiddenRegisters);
-      assert(PossibleRegisters.any() && "No register left to choose from");
+      remove(PossibleRegisters, ImplicitUseAliases);
+      if (!PossibleRegisters.any())
+        return make_error<StringError>(
+            Twine("no available registers:\ncandidates:\n")
+                .concat(debugString(State.getRegInfo(),
+                                    Op.getRegisterAliasing().sourceBits()))
+                .concat("\nforbidden:\n")
+                .concat(debugString(State.getRegInfo(), ForbiddenRegisters))
+                .concat("\nimplicit use:\n")
+                .concat(debugString(State.getRegInfo(), ImplicitUseAliases)),
+            inconvertibleErrorCode());
       const auto RandomReg = randomBit(PossibleRegisters);
       Defs.set(RandomReg);
       Variant.getValueFor(Op) = MCOperand::createReg(RandomReg);
     }
   }
   // And pick random use values that are not reserved and don't alias with defs.
+  // Note that we don't try to avoid using implicit uses explicitly.
   const auto DefAliases = getAliasedBits(State.getRegInfo(), Defs);
   for (const auto &Op : Instr.Operands) {
     if (Op.isReg() && Op.isExplicit() && Op.isUse() && !Op.isMemory()) {
       auto PossibleRegisters = Op.getRegisterAliasing().sourceBits();
       remove(PossibleRegisters, ForbiddenRegisters);
       remove(PossibleRegisters, DefAliases);
+      remove(PossibleRegisters, ImplicitDefAliases);
       assert(PossibleRegisters.any() && "No register left to choose from");
       const auto RandomReg = randomBit(PossibleRegisters);
       Variant.getValueFor(Op) = MCOperand::createReg(RandomReg);
     }
   }
   CT.Info =
       "instruction has no tied variables picking Uses different from defs";
   CT.Instructions.push_back(std::move(Variant));
   instantiateMemoryOperands(CT.ScratchSpacePointerInReg, CT.Instructions);
   return getSingleton(std::move(CT));
 }
 
 constexpr const size_t ParallelSnippetGenerator::kMinNumDifferentAddresses;
 
 } // namespace exegesis
 } // namespace llvm

llvm/unittests/tools/llvm-exegesis/X86/SnippetGeneratorTest.cpp

TEST_F(X86SerialSnippetGeneratorTest,
   auto AllRegisters = State.getRATC().emptyRegisters();
   AllRegisters.flip();
   auto Error =
       Generator.generateCodeTemplates(&Instr, AllRegisters).takeError();
   EXPECT_TRUE((bool)Error);
   consumeError(std::move(Error));
 }
 
-TEST_F(X86SerialSnippetGeneratorTest,
-       AvoidSerializingThroughImplicitRegisters) {
-  // MULX32rr implicitly uses EDX. We should not select that register to avoid
-  // serialization.
-  const unsigned Opcode = X86::MULX32rr;
-  randomGenerator().seed(0); // Initialize seed.
-  const Instruction &Instr = State.getIC().getInstr(Opcode);
-  // Forbid all registers but RDX/EDX/DX/DH/DL. The only option would be to
-  // choose that register, but that would serialize the instruction, so we
-  // should be returning an error.
-  auto AllRegisters = State.getRATC().emptyRegisters();
-  AllRegisters.flip();
-  AllRegisters.reset(X86::RDX);
-  AllRegisters.reset(X86::EDX);
-  AllRegisters.reset(X86::DX);
-  AllRegisters.reset(X86::DH);
-  AllRegisters.reset(X86::DL);
-  auto Error =
-      Generator.generateCodeTemplates(&Instr, AllRegisters).takeError();
-  // FIXME: EXPECT_TRUE + consumeError(std::move(Error)).
-  EXPECT_FALSE((bool)Error);
-}
-
 TEST_F(X86SerialSnippetGeneratorTest, DependencyThroughOtherOpcode) {
   // - CMP64rr
   // - Op0 Explicit Use RegClass(GR64)
   // - Op1 Explicit Use RegClass(GR64)
   // - Op2 Implicit Def Reg(EFLAGS)
   // - Var0 [Op0]
   // - Var1 [Op1]
   const unsigned Opcode = X86::CMP64rr;
TEST_F(X86ParallelSnippetGeneratorTest, MOV16ms) {
   std::vector<BenchmarkCode> Benchmarks;
   auto Err = Generator.generateConfigurations(&Instr, Benchmarks,
                                               State.getRATC().emptyRegisters());
   EXPECT_TRUE((bool)Err);
   EXPECT_THAT(toString(std::move(Err)),
               testing::HasSubstr("no available registers"));
 }
 
+TEST_F(X86ParallelSnippetGeneratorTest,
+       AvoidSerializingThroughImplicitRegisters) {
+  // MULX32rr implicitly uses EDX. We should not select that register to avoid
+  // serialization.
+  const unsigned Opcode = X86::MULX32rr;
+  randomGenerator().seed(0); // Initialize seed.
+  const Instruction &Instr = State.getIC().getInstr(Opcode);
+  // Forbid all registers but RDX/EDX/DX/DH/DL. The only option would be to
+  // choose that register, but that would serialize the instruction, so we
+  // should be returning an error.
+  auto AllRegisters = State.getRATC().emptyRegisters();
+  AllRegisters.flip();
+  AllRegisters.reset(X86::RDX);
+  AllRegisters.reset(X86::EDX);
+  AllRegisters.reset(X86::DX);
+  AllRegisters.reset(X86::DH);
+  AllRegisters.reset(X86::DL);
+  auto Err = Generator.generateCodeTemplates(&Instr, AllRegisters);
+  EXPECT_FALSE((bool)Err);
+  EXPECT_THAT(toString(Err.takeError()),
+              testing::HasSubstr("no available registers"));
+}
+
 class X86FakeSnippetGenerator : public SnippetGenerator {
 public:
   X86FakeSnippetGenerator(const LLVMState &State, const Options &Opts)
       : SnippetGenerator(State, Opts) {}
 
   const Instruction &getInstr(unsigned Opcode) {
     return State.getIC().getInstr(Opcode);
   }