[PATCH] D139283: [llvm-exegesis] parallel snippet generator: avoid Read-After-Write pitfail for instrs w/ tied variables

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

The overall approach sounds good.

FYI, there is an other way to model all this, that we tried at one point: Simply use an SMT solver and generate constraints (LLVM already depends on S3). That was too slow though, so we abandoned it.



================
Comment at: llvm/tools/llvm-exegesis/lib/ParallelSnippetGenerator.cpp:117
 
-static std::vector<InstructionTemplate> generateSnippetUsingStaticRenaming(
+enum class UseRegRandomizationStrategy : uint8_t {
+  PickRandomRegs,
----------------
RKSimon wrote:
> (trivial) Not sure if we need the 'Use' prefix? RegRandomizationStrategy?
+1


================
Comment at: llvm/tools/llvm-exegesis/lib/ParallelSnippetGenerator.cpp:135
+
+const char *getName(UseRegRandomizationStrategy S) {
+  switch (S) {
----------------
[nit] `getDescription` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/ParallelSnippetGenerator.cpp:141
+    return "one unique register for each use position";
+  case UseRegRandomizationStrategy::SingleStaticUseReg:
+    return "reusing the same register for all uses";
----------------
Are there cases when this is worse than the previous option ? I'm under the impression that we could just always do that.


================
Comment at: llvm/tools/llvm-exegesis/lib/ParallelSnippetGenerator.cpp:178
     InstructionTemplate TmpIT = IT;
-    // Find a possible register for each variable in turn, marking the
-    // register as taken.
-    for (size_t VarId = 0; VarId < TiedVariables.size(); ++VarId) {
-      const int NextPossibleReg =
-          PossibleRegsForVar[VarId].find_next(Iterators[VarId]);
-      if (NextPossibleReg <= 0) {
-        return Instructions;
-      }
-      TmpIT.getValueFor(*TiedVariables[VarId]) =
-          MCOperand::createReg(NextPossibleReg);
-      // Bump iterator.
-      Iterators[VarId] = NextPossibleReg;
-      // Prevent other variables from using the register.
-      for (BitVector &OtherPossibleRegs : PossibleRegsForVar) {
-        OtherPossibleRegs.reset(NextPossibleReg);
+    for (const auto &Op : IT.getInstr().Operands) {
+      if (Op.isReg() && Op.isExplicit() && !Op.isMemory() &&
----------------
Can you refactor this in a separate function ? It would make it clearer what state is only read and what state is modified by the loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139283