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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: courbet, RKSimon, gchatelet.
lebedev.ri added a project: LLVM.
Herald added subscribers: mstojanovic, pengfei.
lebedev.ri requested review of this revision.

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 <https://reviews.llvm.org/rGd4d459e7475b4bb0d15280f12ed669342fa5edcd>,
i suspect there may be other buggy results lying around, so we should at least stop producing them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109275

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


Index: llvm/tools/llvm-exegesis/lib/ParallelSnippetGenerator.cpp
===================================================================
--- llvm/tools/llvm-exegesis/lib/ParallelSnippetGenerator.cpp
+++ llvm/tools/llvm-exegesis/lib/ParallelSnippetGenerator.cpp
@@ -186,12 +186,35 @@
     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);
+      remove(PossibleRegisters, ImplicitUseAliases);
       assert(PossibleRegisters.any() && "No register left to choose from");
       const auto RandomReg = randomBit(PossibleRegisters);
       Defs.set(RandomReg);
@@ -199,12 +222,14 @@
     }
   }
   // 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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D109275.370749.patch
Type: text/x-patch
Size: 2790 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210904/0b04d5b4/attachment.bin>