[AMDGPU] Fixed wait count reuse

[rampitec]

Summary

The code reusing existing wait counts is incorrect since it keeps
adding new operands to an old instruction instead of replacing
the immediate. It was also effectively switched off by the condition
that wait count is not an AMDGPU::S_WAITCNT.

Also switched to BuildMI instead of creating instructions directly.

[msearles]

LGTM; what about a test that exercises the code that was effectively turned off (~line 1129)?

[rampitec]

In D42997#999982, @msearles wrote:

LGTM; what about a test that exercises the code that was effectively turned off (~line 1129)?

5 of our existing lit tests hit those lines when enabled and reuse an existing instruction.

[rampitec]

Rebased and merged with rL324440.

llvm/trunk/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

public:
   void addKillWaitBracket(BlockWaitcntBrackets *Bracket) {
     // The waitcnt information is copied because it changes as the block is
     // traversed.
     KillWaitBrackets.push_back(
         llvm::make_unique<BlockWaitcntBrackets>(*Bracket));
   }
 
   bool mayAccessLDSThroughFlat(const MachineInstr &MI) const;
-  MachineInstr *generateSWaitCntInstBefore(MachineInstr &MI,
-                                           BlockWaitcntBrackets *ScoreBrackets);
+  void generateSWaitCntInstBefore(MachineInstr &MI,
   void updateEventWaitCntAfter(MachineInstr &Inst,
                                BlockWaitcntBrackets *ScoreBrackets);
   void mergeInputScoreBrackets(MachineBasicBlock &Block);
   MachineBasicBlock *loopBottom(const MachineLoop *Loop);
   void insertWaitcntInBlock(MachineFunction &MF, MachineBasicBlock &Block);
   void insertWaitcntBeforeCF(MachineBasicBlock &Block, MachineInstr *Inst);
 };
 

 ///  but instructions of different types can complete out of order.
 ///  We rely on this in-order completion
 ///  and simply assign a score to the memory access instructions.
 ///  We keep track of the active "score bracket" to determine
 ///  if an access of a memory read requires an s_waitcnt
 ///  and if so what the value of each counter is.
 ///  The "score bracket" is bound by the lower bound and upper bound
 ///  scores (*_score_LB and *_score_ub respectively).
-MachineInstr *SIInsertWaitcnts::generateSWaitCntInstBefore(
+void SIInsertWaitcnts::generateSWaitCntInstBefore(
     MachineInstr &MI, BlockWaitcntBrackets *ScoreBrackets) {
   // To emit, or not to emit - that's the question!
   // Start with an assumption that there is no need to emit.
   unsigned int EmitSwaitcnt = 0;
-  // s_waitcnt instruction to return; default is NULL.
-  MachineInstr *SWaitInst = nullptr;
   // No need to wait before phi. If a phi-move exists, then the wait should
   // has been inserted before the move. If a phi-move does not exist, then
   // wait should be inserted before the real use. The same is true for
   // sc-merge. It is not a coincident that all these cases correspond to the
   // instructions that are skipped in the assembling loop.
   bool NeedLineMapping = false; // TODO: Check on this.
   if (MI.isDebugValue() &&
       // TODO: any other opcode?
       !NeedLineMapping) {
-    return SWaitInst;
+    return;
   }
 
   // See if an s_waitcnt is forced at block entry, or is needed at
   // program end.
   if (ScoreBrackets->getWaitAtBeginning()) {
     // Note that we have already cleared the state, so we don't need to update
     // it.
     ScoreBrackets->clearWaitAtBeginning();
if (EmitSwaitcnt != 0) {
           }
           ScoreBracket->setRevisitLoop(true);
           DEBUG(dbgs() << "set-revisit: block"
                        << ContainingLoop->getHeader()->getNumber() << '\n';);
         }
       }
 
       // Update an existing waitcount, or make a new one.
-      MachineFunction &MF = *MI.getParent()->getParent();
-      if (OldWaitcnt && OldWaitcnt->getOpcode() != AMDGPU::S_WAITCNT) {
-        SWaitInst = OldWaitcnt;
+      unsigned Enc = AMDGPU::encodeWaitcnt(IV, CntVal[VM_CNT],
+                                           CntVal[EXP_CNT], CntVal[LGKM_CNT]);
+      // We don't (yet) track waitcnts that existed prior to the waitcnt
+      // pass (we just skip over them); because the waitcnt pass is ignorant
+      // of them, it may insert a redundant waitcnt. To avoid this, check
+      // the prev instr. If it and the to-be-inserted waitcnt are the
+      // same, keep the prev waitcnt and skip the insertion. We assume that
+      // whomever. e.g., for memory model, inserted the prev waitcnt really
+      // wants it there.
+      bool insertSWaitInst = true;
+      if (MI.getIterator() != MI.getParent()->begin()) {
+        MachineInstr *MIPrevInst = &*std::prev(MI.getIterator());
+        if (MIPrevInst &&
+            MIPrevInst->getOpcode() == AMDGPU::S_WAITCNT &&
+            MIPrevInst->getOperand(0).getImm() == Enc) {
+          insertSWaitInst = false;
+        }
+      }
+      if (insertSWaitInst) {
+        if (OldWaitcnt && OldWaitcnt->getOpcode() == AMDGPU::S_WAITCNT) {
+          OldWaitcnt->getOperand(0).setImm(Enc);
+          MI.getParent()->insert(MI, OldWaitcnt);
+
+          DEBUG(dbgs() << "updateWaitcntInBlock\n"
+                       << "Old Instr: " << MI << '\n'
+                       << "New Instr: " << *OldWaitcnt << '\n');
         } else {
-        SWaitInst = MF.CreateMachineInstr(TII->get(AMDGPU::S_WAITCNT),
-                                          MI.getDebugLoc());
+            auto SWaitInst = BuildMI(*MI.getParent(), MI.getIterator(),
+                               MI.getDebugLoc(), TII->get(AMDGPU::S_WAITCNT))
+                             .addImm(Enc);
             TrackedWaitcntSet.insert(SWaitInst);
-      }
 
-      const MachineOperand &Op =
-          MachineOperand::CreateImm(AMDGPU::encodeWaitcnt(
-              IV, CntVal[VM_CNT], CntVal[EXP_CNT], CntVal[LGKM_CNT]));
-      SWaitInst->addOperand(MF, Op);
+            DEBUG(dbgs() << "insertWaitcntInBlock\n"
+                         << "Old Instr: " << MI << '\n'
+                         << "New Instr: " << *SWaitInst << '\n');
+        }
+      }
 
       if (CntVal[EXP_CNT] == 0) {
         ScoreBrackets->setMixedExpTypes(false);
       }
     }
   }
-
-  return SWaitInst;
 }
 
 void SIInsertWaitcnts::insertWaitcntBeforeCF(MachineBasicBlock &MBB,
                                              MachineInstr *Waitcnt) {
   if (MBB.empty()) {
     MBB.push_back(Waitcnt);
     return;
   }
if (readsVCCZ(Inst) &&
           ScoreBrackets->hasPendingSMEM()) {
         if (ST->getGeneration() <= SISubtarget::SEA_ISLANDS)
           VCCZBugWorkAround = true;
       }
     }
 
     // Generate an s_waitcnt instruction to be placed before
     // cur_Inst, if needed.
-    MachineInstr *SWaitInst = generateSWaitCntInstBefore(Inst, ScoreBrackets);
-
-    if (SWaitInst) {
-      // We don't (yet) track waitcnts that existed prior to the waitcnt
-      // pass (we just skip over them); because the waitcnt pass is ignorant
-      // of them, it may insert a redundant waitcnt. To avoid this, check
-      // the prev instr. If it and the to-be-inserted waitcnt are the
-      // same, keep the prev waitcnt and skip the insertion. We assume that
-      // whomever. e.g., for memory model, inserted the prev waitcnt really
-      // wants it there.
-      bool insertSWaitInst = true;
-      if (Iter != Block.begin()) {
-        MachineInstr *MIPrevInst = &*std::prev(Iter);
-        if (MIPrevInst &&
-            MIPrevInst->getOpcode() == AMDGPU::S_WAITCNT &&
-            MIPrevInst->getOperand(0).getImm() == SWaitInst->getOperand(0).getImm()) {
-          insertSWaitInst = false;
-        }
-      }
-      if (insertSWaitInst) {
-        Block.insert(Inst, SWaitInst);
-        if (ScoreBrackets->getWaitcnt() != SWaitInst) {
-          DEBUG(dbgs() << "insertWaitcntInBlock\n"
-                       << "Old Instr: " << Inst << '\n'
-                       << "New Instr: " << *SWaitInst << '\n';);
-        }
-      }
-    }
+    generateSWaitCntInstBefore(Inst, ScoreBrackets);
 
     updateEventWaitCntAfter(Inst, ScoreBrackets);
 
 #if 0 // TODO: implement resource type check controlled by options with ub = LB.
     // If this instruction generates a S_SETVSKIP because it is an
     // indexed resource, and we are on Tahiti, then it will also force
     // an S_WAITCNT vmcnt(0)
     if (RequireCheckResourceType(Inst, context)) {
       // Force the score to as if an S_WAITCNT vmcnt(0) is emitted.
       ScoreBrackets->setScoreLB(VM_CNT,
       ScoreBrackets->getScoreUB(VM_CNT));
     }
 #endif
 
     ScoreBrackets->clearWaitcnt();
 
-    if (SWaitInst) {
-      DEBUG({ SWaitInst->print(dbgs() << '\n'); });
-    }
     DEBUG({
       Inst.print(dbgs());
       ScoreBrackets->dump();
     });
 
     // Check to see if this is a GWS instruction. If so, and if this is CI or
     // VI, then the generated code sequence will include an S_WAITCNT 0.
     // TODO: Are these the only GWS instructions?