[AMDGPU][SetWavePriority] Fix dealing with MBBInfo records.

[kosarev]

Summary

Happened earlier than I anticipated. :)

[kosarev]

llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp

150–152

These potentially invalidate Info.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp

150–152

Doesn't this imply it's reading uncomputed values for the successors? Don't those need to be computed for this to do something sensible?

[kosarev]

llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp

150–152

I understand where the edge to a successor is a back edge, we will examine its MBBInfo here before the main post_order(&MF) loop had a chance to compute it. Logic-wise that itself is not a problem because we ignore loops, but since such successors are new to MBBInfos, this invalidates Info.

[kosarev]

Ping.

[arsenm]

LGTM. I really hate insert by index operator.

llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp

131

Move &Info down here to avoid 2 map lookups?

150–152

Should this not ignore loops?

154

Ditto?

[kosarev]

Updated as suggested.

[kosarev]

In D134726#3826821, @arsenm wrote:

LGTM. I really hate insert by index operator.

I still think the initial implementation with non-relocatable MBBInfos would work better here.

Thanks for reviewing.

llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp

150–152

That's something to be tried and seen. We don't know it yet.

llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp

bool AMDGPUSetWavePriority::runOnMachineFunction(MachineFunction &MF) {
   // Find VMEM loads that may be executed before long-enough sequences of
   // VALU instructions. We currently assume that backedges/loops, branch
   // probabilities and other details can be ignored, so we essentially
   // determine the largest number of VALU instructions along every
   // possible path from the start of the function that may potentially be
   // executed provided no backedge is ever taken.
   MBBInfoSet MBBInfos;
   for (MachineBasicBlock *MBB : post_order(&MF)) {
-    MBBInfo &Info = MBBInfos[MBB];
     bool AtStart = true;
     unsigned MaxNumVALUInstsInMiddle = 0;
     unsigned NumVALUInstsAtEnd = 0;
     for (MachineInstr &MI : *MBB) {
       if (isVMEMLoad(MI)) {
         AtStart = false;
-        Info.NumVALUInstsAtStart = 0;
+        MBBInfos[MBB].NumVALUInstsAtStart = 0;
         MaxNumVALUInstsInMiddle = 0;
         NumVALUInstsAtEnd = 0;
-        Info.LastVMEMLoad = &MI;
+        MBBInfos[MBB].LastVMEMLoad = &MI;
       } else if (SIInstrInfo::isDS(MI)) {
         AtStart = false;
         MaxNumVALUInstsInMiddle =
             std::max(MaxNumVALUInstsInMiddle, NumVALUInstsAtEnd);
         NumVALUInstsAtEnd = 0;
       } else if (SIInstrInfo::isVALU(MI)) {
         if (AtStart)
-          ++Info.NumVALUInstsAtStart;
+          ++MBBInfos[MBB].NumVALUInstsAtStart;
         ++NumVALUInstsAtEnd;
       }
     }
 
     bool SuccsMayReachVMEMLoad = false;
     unsigned NumFollowingVALUInsts = 0;
     for (const MachineBasicBlock *Succ : MBB->successors()) {
       SuccsMayReachVMEMLoad |= MBBInfos[Succ].MayReachVMEMLoad;
       NumFollowingVALUInsts =
           std::max(NumFollowingVALUInsts, MBBInfos[Succ].NumVALUInstsAtStart);
     }
     if (AtStart)
-      Info.NumVALUInstsAtStart += NumFollowingVALUInsts;
+      MBBInfos[MBB].NumVALUInstsAtStart += NumFollowingVALUInsts;
     NumVALUInstsAtEnd += NumFollowingVALUInsts;
 
     unsigned MaxNumVALUInsts =
         std::max(MaxNumVALUInstsInMiddle, NumVALUInstsAtEnd);
-    Info.MayReachVMEMLoad =
+    MBBInfos[MBB].MayReachVMEMLoad =
         SuccsMayReachVMEMLoad ||
-        (Info.LastVMEMLoad && MaxNumVALUInsts >= VALUInstsThreshold);
+        (MBBInfos[MBB].LastVMEMLoad && MaxNumVALUInsts >= VALUInstsThreshold);
   }
 
   MachineBasicBlock &Entry = MF.front();
   if (!MBBInfos[&Entry].MayReachVMEMLoad)
     return false;
 
   // Raise the priority at the beginning of the shader.
   MachineBasicBlock::iterator I = Entry.begin(), E = Entry.end();