[globalisel][tablegen] Do not merge memoperands from instructions that weren't in the match.

[dsanders]

Summary

Fix a bug discovered in an out-of-tree target where memoperands from
pseudo-instructions that weren't part of the match were being merged into the
result instructions as part of GIR_MergeMemOperands.

This bug was caused by a change to the handling of State.MIs between rules when
the state machine tables were fused into a single table. Previously, each rule
would reset State.MIs using State.MIs.resize(1) but this is no longer done, as a
result stale data is occasionally left in some elements of State.MIs. Most
opcodes aren't affected by this but GIR_MergeMemOperands merges all memoperands
from the intructions recorded in State.MIs into the result instruction.

Suppose for example, we processed but rejected the following pattern:

(signextend (load x))

at this point, State.MIs contains the signextend and the load. Now suppose we
process and accept this pattern:

(add x, y)

at this point, State.MIs contains the add as well as the (now irrelevant) load.
When GIR_MergeMemOperands is processed, the memoperands from that irrelevant
load will be merged into the result instruction even though it was not part of
the match.

Bringing back the State.MIs.resize(1) would fix the problem but it would limit
our ability to optimize the table in the future. Instead, this patch fixes the
problem by explicitly stating which instructions should be merged into the result.

There's no direct test case in this commit because a test case would be very brittle.
However, at the time of writing this should fix the failures in
http://green.lab.llvm.org/green/job/Compiler_Verifiers_GlobalISEL/ as well as a
failure in test/CodeGen/ARM/GlobalISel/arm-isel.ll when expensive checks are enabled.

[dsanders]

It's been pointed out to me that one of the bots (http://green.lab.llvm.org/green/job/Compiler_Verifiers_GlobalISEL/) is red because of the bug this fixes. That being the case, I'm going to commit this and we can post-commit review it.

[fhahn]

I'll commit D36151 after this one goes in, so we see if this change fixes the buildbot

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h

377

Does this mean that now we are advancing CurrentIdx till the end? Could this have an impact on further actions after GIR_MergeMemOperands?

[dsanders]

In D36094#828809, @fhahn wrote:

I'll commit D36151 after this one goes in, so we see if this change fixes the buildbot

Thanks. I'll commit it soon, I'm just re-running 'ninja check' at the moment.

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h

377

Until the end of the variable-length MergeInsnID list which is indicated with a -1. After breaking out of this loop, CurrentIdx will be the index for the next GIR_* opcode.

[fhahn]

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h

377

Ah I thought it was something like that.

It might be slightly clearer if we would use a defined constant like END_VARARGS or something instead of -1, but that may be just because I have not much knowledge about GlobalISel

[dsanders]

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h

377

Ok, I'll make this change before the commit

include/llvm/CodeGen/GlobalISel/InstructionSelector.h

enum {
   /// - RCEnum - Register class enumeration value
   GIR_ConstrainOperandRC,
   /// Constrain an instructions operands according to the instruction
   /// description.
   /// - InsnID - Instruction ID to modify
   GIR_ConstrainSelectedInstOperands,
   /// Merge all memory operands into instruction.
   /// - InsnID - Instruction ID to modify
+  /// - MergeInsnID... - One or more Instruction ID to merge into the result.
+  /// - -1 - Terminates the list of instructions to merge.
   GIR_MergeMemOperands,
   /// Erase from parent.
   /// - InsnID - Instruction ID to erase
   GIR_EraseFromParent,
 
   /// A successful emission
   GIR_Done,
 };

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h

case GIR_ConstrainSelectedInstOperands: {
       DEBUG(dbgs() << CurrentIdx
                    << ": GIR_ConstrainSelectedInstOperands(OutMIs[" << InsnID
                    << "])\n");
       break;
     }
     case GIR_MergeMemOperands: {
       int64_t InsnID = MatchTable[CurrentIdx++];
       assert(OutMIs[InsnID] && "Attempted to add to undefined instruction");
-      for (const auto *FromMI : State.MIs)
-        for (const auto &MMO : FromMI->memoperands())
-          OutMIs[InsnID].addMemOperand(MMO);
+
       DEBUG(dbgs() << CurrentIdx << ": GIR_MergeMemOperands(OutMIs[" << InsnID
-                   << "])\n");
+                   << "]");
+      int64_t MergeInsnID = -1;
+      while ((MergeInsnID = MatchTable[CurrentIdx++]) != -1) {
+        DEBUG(dbgs() << ", MIs[" << MergeInsnID << "]");
+        for (const auto &MMO : State.MIs[MergeInsnID]->memoperands())
+          OutMIs[InsnID].addMemOperand(MMO);
+      }
+      DEBUG(dbgs() << ")\n");
       break;
     }
     case GIR_EraseFromParent: {
       int64_t InsnID = MatchTable[CurrentIdx++];
       assert(State.MIs[InsnID] &&
              "Attempted to erase an undefined instruction");
       State.MIs[InsnID]->eraseFromParent();
       DEBUG(dbgs() << CurrentIdx << ": GIR_EraseFromParent(MIs[" << InsnID

utils/TableGen/GlobalISelEmitter.cpp

class RuleMatcher {
   /// extended to support multiple positions to support div/rem fusion or
   /// load-multiple instructions.
   std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
 
   /// A list of actions that need to be taken when all predicates in this rule
   /// have succeeded.
   std::vector<std::unique_ptr<MatchAction>> Actions;
 
+  typedef std::map<const InstructionMatcher *, unsigned>
+      DefinedInsnVariablesMap;
   /// A map of instruction matchers to the local variables created by
   /// emitCaptureOpcodes().
-  std::map<const InstructionMatcher *, unsigned> InsnVariableIDs;
+  DefinedInsnVariablesMap InsnVariableIDs;
 
   /// ID for the next instruction variable defined with defineInsnVar()
   unsigned NextInsnVarID;
 
   std::vector<Record *> RequiredFeatures;
 
 public:
   RuleMatcher()
public:
 
   /// Define an instruction without emitting any code to do so.
   /// This is used for the root of the match.
   unsigned implicitlyDefineInsnVar(const InstructionMatcher &Matcher);
   /// Define an instruction and emit corresponding state-machine opcodes.
   unsigned defineInsnVar(MatchTable &Table, const InstructionMatcher &Matcher,
                          unsigned InsnVarID, unsigned OpIdx);
   unsigned getInsnVarID(const InstructionMatcher &InsnMatcher) const;
+  DefinedInsnVariablesMap::const_iterator defined_insn_vars_begin() const {
+    return InsnVariableIDs.begin();
+  }
+  DefinedInsnVariablesMap::const_iterator defined_insn_vars_end() const {
+    return InsnVariableIDs.end();
+  }
+  iterator_range<typename DefinedInsnVariablesMap::const_iterator>
+  defined_insn_vars() const {
+    return make_range(defined_insn_vars_begin(), defined_insn_vars_end());
+  }
 
   void emitCaptureOpcodes(MatchTable &Table);
 
   void emit(MatchTable &Table);
 
   /// Compare the priority of this object and B.
   ///
   /// Returns true if this object is more important than B.
Table << MatchTable::Opcode("GIR_BuildMI") << MatchTable::Comment("InsnID")
           << MatchTable::IntValue(InsnID) << MatchTable::Comment("Opcode")
           << MatchTable::NamedValue(I->Namespace, I->TheDef->getName())
           << MatchTable::LineBreak;
     for (const auto &Renderer : OperandRenderers)
       Renderer->emitRenderOpcodes(Table, Rule);
 
     Table << MatchTable::Opcode("GIR_MergeMemOperands")
           << MatchTable::Comment("InsnID") << MatchTable::IntValue(InsnID)
+          << MatchTable::Comment("MergeInsnID's");
+    // Emit the ID's for all the instructions that are matched by this rule.
+    // TODO: Limit this to matched instructions that mayLoad/mayStore or have
+    //       some other means of having a memoperand. Also limit this to emitted
+    //       instructions that expect to have a memoperand too. For example,
+    //       (G_SEXT (G_LOAD x)) that results in separate load and sign-extend
+    //       instructions shouldn't put the memoperand on the sign-extend since
+    //       it has no effect there.
+    for (const auto &IDMatcherPair : Rule.defined_insn_vars())
+      Table << MatchTable::IntValue(IDMatcherPair.second);
+    Table << MatchTable::Comment("EOL") << MatchTable::IntValue(-1)
           << MatchTable::LineBreak << MatchTable::Opcode("GIR_EraseFromParent")
           << MatchTable::Comment("InsnID")
           << MatchTable::IntValue(RecycleInsnID) << MatchTable::LineBreak;
   }
 };
 
 /// Generates code to constrain the operands of an output instruction to the
 /// register classes specified by the definition of that instruction.