[llvm-exegesis] Finish plumbing the `Config` field.

[courbet]

Summary

Right now there are no snippet generators that emit the Config Field,
but I plan to add it to investigate LEA operands for PR32326.

What was broken was:

• Config Was not propagated up until the BenchmarkResult::Key.
• Clustering should really consider different configs as measuring different things, so we should stabilize on (Opcode, Config) instead of just Opcode.

[courbet]

Rebase

[gchatelet]

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

241

Not related to this patch but why a question mark here?

247

How about factoring Tie:

inline auto Tie() const { return std::tie(Opcode, *Config); }
bool operator<(const A &O) const { return Tie() < O.Tie(); }
bool operator==(const A &O) const { return Tie() == O.Tie(); }

295

maybe add the != operator to the struct, with the Tie() function it's a no brainer.

[courbet]

Addres comments

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

241

Not sure, but that doe snot bother me too much :)

[courbet]

remove spurious edit

llvm/test/tools/llvm-exegesis/X86/analysis-cluster-stabilization-config.test

 # RUN: llvm-exegesis -mode=analysis -benchmarks-file=%s -analysis-inconsistencies-output-file=- -analysis-clustering-epsilon=0.5 -analysis-inconsistency-epsilon=0.5 -analysis-display-unstable-clusters -analysis-numpoints=1 | FileCheck -check-prefixes=CHECK-UNSTABLE %s
 
 # We have two measurements with different measurements for SQRTSSr, but they
 # have different configs, so they should not be placed in the same cluster by
 # stabilization.
 
-# CHECK-UNSTABLE: SQRTSSr
-# CHECK-UNSTABLE: SQRTSSr
+# CHECK-UNSTABLE-NOT: SQRTSSr
 
 ---
 mode:            latency
 key:
   instructions:
     - 'SQRTSSr XMM11 XMM11'
   config:          'config1'
   register_initial_values:

llvm/tools/llvm-exegesis/lib/BenchmarkCode.h

 //===-- BenchmarkCode.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_EXEGESIS_BENCHMARKCODE_H
 #define LLVM_TOOLS_LLVM_EXEGESIS_BENCHMARKCODE_H
 
-#include "RegisterValue.h"
+#include "BenchmarkResult.h"
 #include "llvm/MC/MCInst.h"
 #include <string>
 #include <vector>
 
 namespace llvm {
 namespace exegesis {
 
 // A collection of instructions that are to be assembled, executed and measured.
 struct BenchmarkCode {
-  // The sequence of instructions that are to be repeated.
-  std::vector<llvm::MCInst> Instructions;
-
-  // Before the code is executed some instructions are added to setup the
-  // registers initial values.
-  std::vector<RegisterValue> RegisterInitialValues;
+  InstructionBenchmarkKey Key;
 
   // We also need to provide the registers that are live on entry for the
   // assembler to generate proper prologue/epilogue.
   std::vector<unsigned> LiveIns;
 
   // Informations about how this configuration was built.
   std::string Info;
 };
 
 } // namespace exegesis
 } // namespace llvm
 
 #endif // LLVM_TOOLS_LLVM_EXEGESIS_BENCHMARKCODE_H

llvm/tools/llvm-exegesis/lib/BenchmarkResult.h

 /// Defines classes to represent measurements and serialize/deserialize them to
 //  Yaml.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_EXEGESIS_BENCHMARKRESULT_H
 #define LLVM_TOOLS_LLVM_EXEGESIS_BENCHMARKRESULT_H
 
-#include "BenchmarkCode.h"
 #include "LlvmState.h"
+#include "RegisterValue.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstBuilder.h"
 #include "llvm/Support/YAMLTraits.h"
 #include <limits>
 #include <string>
 #include <unordered_map>

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

 namespace exegesis {
 
 BenchmarkRunner::BenchmarkRunner(const LLVMState &State,
                                  InstructionBenchmark::ModeE Mode)
     : State(State), Mode(Mode), Scratch(std::make_unique<ScratchSpace>()) {}
 
 BenchmarkRunner::~BenchmarkRunner() = default;
 
-
 namespace {
 class FunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
 public:
   FunctionExecutorImpl(const LLVMState &State,
                        llvm::object::OwningBinary<llvm::object::ObjectFile> Obj,
                        BenchmarkRunner::ScratchSpace *Scratch)
       : Function(State.createTargetMachine(), std::move(Obj)),
         Scratch(Scratch) {}
InstructionBenchmark BenchmarkRunner::runConfiguration(
   InstructionBenchmark InstrBenchmark;
   InstrBenchmark.Mode = Mode;
   InstrBenchmark.CpuName = State.getTargetMachine().getTargetCPU();
   InstrBenchmark.LLVMTriple =
       State.getTargetMachine().getTargetTriple().normalize();
   InstrBenchmark.NumRepetitions = NumRepetitions;
   InstrBenchmark.Info = BC.Info;
 
-  const std::vector<llvm::MCInst> &Instructions = BC.Instructions;
+  const std::vector<llvm::MCInst> &Instructions = BC.Key.Instructions;
 
-  InstrBenchmark.Key.Instructions = Instructions;
-  InstrBenchmark.Key.RegisterInitialValues = BC.RegisterInitialValues;
+  InstrBenchmark.Key = BC.Key;
 
   // Assemble at least kMinInstructionsForSnippet instructions by repeating the
   // snippet for debug/analysis. This is so that the user clearly understands
   // that the inside instructions are repeated.
   constexpr const int kMinInstructionsForSnippet = 16;
   {
     llvm::SmallString<0> Buffer;
     llvm::raw_svector_ostream OS(Buffer);
-    assembleToStream(
-        State.getExegesisTarget(), State.createTargetMachine(), BC.LiveIns,
-        BC.RegisterInitialValues,
-        Repetitor.Repeat(BC.Instructions, kMinInstructionsForSnippet), OS);
+    assembleToStream(State.getExegesisTarget(), State.createTargetMachine(),
+                     BC.LiveIns, BC.Key.RegisterInitialValues,
+                     Repetitor.Repeat(Instructions, kMinInstructionsForSnippet),
+                     OS);
     const ExecutableFunction EF(State.createTargetMachine(),
                                 getObjectFromBuffer(OS.str()));
     const auto FnBytes = EF.getFunctionBytes();
     InstrBenchmark.AssembledSnippet.assign(FnBytes.begin(), FnBytes.end());
   }
 
   // Assemble NumRepetitions instructions repetitions of the snippet for
   // measurements.
   const auto Filler =
-      Repetitor.Repeat(BC.Instructions, InstrBenchmark.NumRepetitions);
+      Repetitor.Repeat(Instructions, InstrBenchmark.NumRepetitions);
 
   llvm::object::OwningBinary<llvm::object::ObjectFile> ObjectFile;
   if (DumpObjectToDisk) {
     auto ObjectFilePath = writeObjectFile(BC, Filler);
     if (llvm::Error E = ObjectFilePath.takeError()) {
       InstrBenchmark.Error = llvm::toString(std::move(E));
       return InstrBenchmark;
     }
     llvm::outs() << "Check generated assembly with: /usr/bin/objdump -d "
                  << *ObjectFilePath << "\n";
     ObjectFile = getObjectFromFile(*ObjectFilePath);
   } else {
     llvm::SmallString<0> Buffer;
     llvm::raw_svector_ostream OS(Buffer);
     assembleToStream(State.getExegesisTarget(), State.createTargetMachine(),
-                     BC.LiveIns, BC.RegisterInitialValues, Filler, OS);
+                     BC.LiveIns, BC.Key.RegisterInitialValues, Filler, OS);
     ObjectFile = getObjectFromBuffer(OS.str());
   }
 
   const FunctionExecutorImpl Executor(State, std::move(ObjectFile),
                                       Scratch.get());
   auto Measurements = runMeasurements(Executor);
   if (llvm::Error E = Measurements.takeError()) {
     InstrBenchmark.Error = llvm::toString(std::move(E));
     return InstrBenchmark;
   }
   InstrBenchmark.Measurements = std::move(*Measurements);
   assert(InstrBenchmark.NumRepetitions > 0 && "invalid NumRepetitions");
   for (BenchmarkMeasure &BM : InstrBenchmark.Measurements) {
     // Scale the measurements by instruction.
     BM.PerInstructionValue /= InstrBenchmark.NumRepetitions;
     // Scale the measurements by snippet.
-    BM.PerSnippetValue *= static_cast<double>(BC.Instructions.size()) /
+    BM.PerSnippetValue *= static_cast<double>(Instructions.size()) /
                           InstrBenchmark.NumRepetitions;
   }
 
   return InstrBenchmark;
 }
 
 llvm::Expected<std::string>
 BenchmarkRunner::writeObjectFile(const BenchmarkCode &BC,
                                  const FillFunction &FillFunction) const {
   int ResultFD = 0;
   llvm::SmallString<256> ResultPath;
   if (llvm::Error E = llvm::errorCodeToError(llvm::sys::fs::createTemporaryFile(
           "snippet", "o", ResultFD, ResultPath)))
     return std::move(E);
   llvm::raw_fd_ostream OFS(ResultFD, true /*ShouldClose*/);
   assembleToStream(State.getExegesisTarget(), State.createTargetMachine(),
-                   BC.LiveIns, BC.RegisterInitialValues, FillFunction, OFS);
+                   BC.LiveIns, BC.Key.RegisterInitialValues, FillFunction, OFS);
   return ResultPath.str();
 }
 
 BenchmarkRunner::FunctionExecutor::~FunctionExecutor() {}
 
 } // namespace exegesis
 } // namespace llvm

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

 // characteristics, depending on their arguments. And thus, if we do several
 // benchmarks of the same instruction Opcode, we may end up with *different*
 // performance characteristics measurements. And when we then do clustering,
 // these several benchmarks of the same instruction Opcode may end up being
 // clustered into *different* clusters. This is not great for further analysis.
 // We shall find every opcode with benchmarks not in just one cluster, and move
 // *all* the benchmarks of said Opcode into one new unstable cluster per Opcode.
 void InstructionBenchmarkClustering::stabilize(unsigned NumOpcodes) {
-  // Given an instruction Opcode, in which clusters do benchmarks of this
-  // instruction lie? Normally, they all should be in the same cluster.
-  std::vector<llvm::SmallSet<ClusterId, 1>> OpcodeToClusterIDs;
-  OpcodeToClusterIDs.resize(NumOpcodes);
-  // The list of opcodes that have more than one cluster.
-  llvm::SetVector<size_t> UnstableOpcodes;
-  // Populate OpcodeToClusterIDs and UnstableOpcodes data structures.
+  // Given an instruction Opcode and Config, in which clusters do benchmarks of
+  // this instruction lie ? Normally, they all should be in the same cluster.
+  struct OpcodeAndConfig {
+    explicit OpcodeAndConfig(const InstructionBenchmark &IB)
+        : Opcode(IB.keyInstruction().getOpcode()), Config(&IB.Key.Config) {}
+    unsigned Opcode;
+    const std::string *Config;
+
+    auto Tie() const -> auto { return std::tie(Opcode, *Config); }
+
+    bool operator<(const OpcodeAndConfig &O) const { return Tie() < O.Tie(); }
+    bool operator!=(const OpcodeAndConfig &O) const { return Tie() != O.Tie(); }
+  };
+  std::map<OpcodeAndConfig, llvm::SmallSet<ClusterId, 1>>
+      OpcodeConfigToClusterIDs;
+  // Populate OpcodeConfigToClusterIDs and UnstableOpcodes data structures.
   assert(ClusterIdForPoint_.size() == Points_.size() && "size mismatch");
   for (const auto &Point : zip(Points_, ClusterIdForPoint_)) {
     const ClusterId &ClusterIdOfPoint = std::get<1>(Point);
     if (!ClusterIdOfPoint.isValid())
       continue; // Only process fully valid clusters.
-    const unsigned Opcode = std::get<0>(Point).keyInstruction().getOpcode();
-    assert(Opcode < NumOpcodes && "NumOpcodes is incorrect (too small)");
+    const OpcodeAndConfig Key(std::get<0>(Point));
     llvm::SmallSet<ClusterId, 1> &ClusterIDsOfOpcode =
-        OpcodeToClusterIDs[Opcode];
+        OpcodeConfigToClusterIDs[Key];
     ClusterIDsOfOpcode.insert(ClusterIdOfPoint);
-    // Is there more than one ClusterID for this opcode?.
-    if (ClusterIDsOfOpcode.size() < 2)
-      continue; // If not, then at this moment this Opcode is stable.
-    // Else let's record this unstable opcode for future use.
-    UnstableOpcodes.insert(Opcode);
-  }
-  assert(OpcodeToClusterIDs.size() == NumOpcodes && "sanity check");
-
-  // We know with how many [new] clusters we will end up with.
-  const auto NewTotalClusterCount = Clusters_.size() + UnstableOpcodes.size();
-  Clusters_.reserve(NewTotalClusterCount);
-  for (const size_t UnstableOpcode : UnstableOpcodes.getArrayRef()) {
+  }
+
+  for (const auto &OpcodeConfigToClusterID : OpcodeConfigToClusterIDs) {
     const llvm::SmallSet<ClusterId, 1> &ClusterIDs =
-        OpcodeToClusterIDs[UnstableOpcode];
-    assert(ClusterIDs.size() > 1 &&
-           "Should only have Opcodes with more than one cluster.");
+        OpcodeConfigToClusterID.second;
+    const OpcodeAndConfig &Key = OpcodeConfigToClusterID.first;
+    // We only care about unstable instructions.
+    if (ClusterIDs.size() < 2)
+      continue;
 
     // Create a new unstable cluster, one per Opcode.
     Clusters_.emplace_back(ClusterId::makeValidUnstable(Clusters_.size()));
     Cluster &UnstableCluster = Clusters_.back();
     // We will find *at least* one point in each of these clusters.
     UnstableCluster.PointIndices.reserve(ClusterIDs.size());
 
     // Go through every cluster which we recorded as containing benchmarks
     // of this UnstableOpcode. NOTE: we only recorded valid clusters.
     for (const ClusterId &CID : ClusterIDs) {
       assert(CID.isValid() &&
              "We only recorded valid clusters, not noise/error clusters.");
       Cluster &OldCluster = Clusters_[CID.getId()]; // Valid clusters storage.
       // Within each cluster, go through each point, and either move it to the
       // new unstable cluster, or 'keep' it.
       // In this case, we'll reshuffle OldCluster.PointIndices vector
       // so that all the points that are *not* for UnstableOpcode are first,
       // and the rest of the points is for the UnstableOpcode.
       const auto it = std::stable_partition(
           OldCluster.PointIndices.begin(), OldCluster.PointIndices.end(),
-          [this, UnstableOpcode](size_t P) {
-            return Points_[P].keyInstruction().getOpcode() != UnstableOpcode;
+          [this, &Key](size_t P) {
+            return OpcodeAndConfig(Points_[P]) != Key;
           });
       assert(std::distance(it, OldCluster.PointIndices.end()) > 0 &&
              "Should have found at least one bad point");
       // Mark to-be-moved points as belonging to the new cluster.
       std::for_each(it, OldCluster.PointIndices.end(),
                     [this, &UnstableCluster](size_t P) {
                       ClusterIdForPoint_[P] = UnstableCluster.Id;
                     });
       // Actually append to-be-moved points to the new cluster.
       UnstableCluster.PointIndices.insert(UnstableCluster.PointIndices.end(),
                                           it, OldCluster.PointIndices.end());
       // And finally, remove "to-be-moved" points form the old cluster.
       OldCluster.PointIndices.erase(it, OldCluster.PointIndices.end());
       // Now, the old cluster may end up being empty, but let's just keep it
       // in whatever state it ended up. Purging empty clusters isn't worth it.
     };
     assert(UnstableCluster.PointIndices.size() > 1 &&
            "New unstable cluster should end up with more than one point.");
     assert(UnstableCluster.PointIndices.size() >= ClusterIDs.size() &&
            "New unstable cluster should end up with no less points than there "
            "was clusters");
   }
-  assert(Clusters_.size() == NewTotalClusterCount && "sanity check");
 }
 
 llvm::Expected<InstructionBenchmarkClustering>
 InstructionBenchmarkClustering::create(
     const std::vector<InstructionBenchmark> &Points, const ModeE Mode,
     const size_t DbscanMinPts, const double AnalysisClusteringEpsilon,
     llvm::Optional<unsigned> NumOpcodes) {
   InstructionBenchmarkClustering Clustering(

llvm/tools/llvm-exegesis/lib/CodeTemplate.h

struct CodeTemplate {
   CodeTemplate() = default;
 
   CodeTemplate(CodeTemplate &&);            // default
   CodeTemplate &operator=(CodeTemplate &&); // default
   CodeTemplate(const CodeTemplate &) = delete;
   CodeTemplate &operator=(const CodeTemplate &) = delete;
 
   ExecutionMode Execution = ExecutionMode::UNKNOWN;
+  // See InstructionBenchmarkKey.::Config.
+  std::string Config;
   // Some information about how this template has been created.
   std::string Info;
   // The list of the instructions for this template.
   std::vector<InstructionTemplate> Instructions;
   // If the template uses the provided scratch memory, the register in which
   // the pointer to this memory is passed in to the function.
   unsigned ScratchSpacePointerInReg = 0;
 };
 
 } // namespace exegesis
 } // namespace llvm
 
 #endif // LLVM_TOOLS_LLVM_EXEGESIS_CODETEMPLATE_H

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

explicit BenchmarkCodeStreamer(MCContext *Context,
                                  const MCRegisterInfo *TheRegInfo,
                                  BenchmarkCode *Result)
       : MCStreamer(*Context), RegInfo(TheRegInfo), Result(Result) {}
 
   // Implementation of the MCStreamer interface. We only care about
   // instructions.
   void EmitInstruction(const MCInst &Instruction,
                        const MCSubtargetInfo &STI) override {
-    Result->Instructions.push_back(Instruction);
+    Result->Key.Instructions.push_back(Instruction);
   }
 
   // Implementation of the AsmCommentConsumer.
   void HandleComment(SMLoc Loc, StringRef CommentText) override {
     CommentText = CommentText.trim();
     if (!CommentText.consume_front("LLVM-EXEGESIS-"))
       return;
     if (CommentText.consume_front("DEFREG")) {
if (CommentText.consume_front("DEFREG")) {
         errs() << "unknown register '" << Parts[0]
                << "' in 'LLVM-EXEGESIS-DEFREG " << CommentText << "'\n";
         ++InvalidComments;
         return;
       }
       const StringRef HexValue = Parts[1].trim();
       RegVal.Value = APInt(
           /* each hex digit is 4 bits */ HexValue.size() * 4, HexValue, 16);
-      Result->RegisterInitialValues.push_back(std::move(RegVal));
+      Result->Key.RegisterInitialValues.push_back(std::move(RegVal));
       return;
     }
     if (CommentText.consume_front("LIVEIN")) {
       // LLVM-EXEGESIS-LIVEIN <reg>
       const auto RegName = CommentText.ltrim();
       if (unsigned Reg = findRegisterByName(RegName))
         Result->LiveIns.push_back(Reg);
       else {

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

if (auto E = generateCodeTemplates(Instr, ForbiddenRegs)) {
     std::vector<BenchmarkCode> Output;
     for (CodeTemplate &CT : E.get()) {
       // TODO: Generate as many BenchmarkCode as needed.
       {
         BenchmarkCode BC;
         BC.Info = CT.Info;
         for (InstructionTemplate &IT : CT.Instructions) {
           randomizeUnsetVariables(State.getExegesisTarget(), ForbiddenRegs, IT);
-          BC.Instructions.push_back(IT.build());
+          BC.Key.Instructions.push_back(IT.build());
         }
         if (CT.ScratchSpacePointerInReg)
           BC.LiveIns.push_back(CT.ScratchSpacePointerInReg);
-        BC.RegisterInitialValues =
+        BC.Key.RegisterInitialValues =
             computeRegisterInitialValues(CT.Instructions);
+        BC.Key.Config = CT.Config;
         Output.push_back(std::move(BC));
       }
     }
     return Output;
   } else
     return E.takeError();
 }
 

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

auto Snippets = TestCommon(R"(
     # LLVM-EXEGESIS-DEFREG SIL 0
     # LLVM-EXEGESIS-LIVEIN RDI
     # LLVM-EXEGESIS-LIVEIN DL
     incq %rax
   )");
   EXPECT_FALSE((bool)Snippets.takeError());
   ASSERT_THAT(*Snippets, SizeIs(1));
   const auto &Snippet = (*Snippets)[0];
-  ASSERT_THAT(Snippet.Instructions, ElementsAre(HasOpcode(X86::INC64r)));
-  ASSERT_THAT(Snippet.RegisterInitialValues,
+  ASSERT_THAT(Snippet.Key.Instructions, ElementsAre(HasOpcode(X86::INC64r)));
+  ASSERT_THAT(Snippet.Key.RegisterInitialValues,
               ElementsAre(RegisterInitialValueIs(X86::RAX, 15),
                           RegisterInitialValueIs(X86::SIL, 0)));
   ASSERT_THAT(Snippet.LiveIns, ElementsAre(X86::RDI, X86::DL));
 }
 
 TEST_F(X86SnippetFileTest, BadDefregParam) {
   auto Error = TestCommon(R"(
     # LLVM-EXEGESIS-DEFREG DOESNOEXIST 0