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

[kosarev]

Summary

Happened earlier than I anticipated. :)

[kosarev]

llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp

151–153

These potentially invalidate Info.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp

151–153

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

151–153

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

132

Move &Info down here to avoid 2 map lookups?

151–153

Should this not ignore loops?

156

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

151–153

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;
+        MBBInfo &Info = MBBInfos[MBB];
         Info.NumVALUInstsAtStart = 0;
         MaxNumVALUInstsInMiddle = 0;
         NumVALUInstsAtEnd = 0;
         Info.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);
     }
+    MBBInfo &Info = MBBInfos[MBB];
     if (AtStart)
       Info.NumVALUInstsAtStart += NumFollowingVALUInsts;
     NumVALUInstsAtEnd += NumFollowingVALUInsts;
 
     unsigned MaxNumVALUInsts =
         std::max(MaxNumVALUInstsInMiddle, NumVALUInstsAtEnd);
     Info.MayReachVMEMLoad =
         SuccsMayReachVMEMLoad ||
         (Info.LastVMEMLoad && MaxNumVALUInsts >= VALUInstsThreshold);