[PATCH] D134726: [AMDGPU][SetWavePriority] Fix dealing with MBBInfo records.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added a comment.
This revision is now accepted and ready to land.

LGTM. I really hate insert by index operator.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:131
         AtStart = false;
-        Info.NumVALUInstsAtStart = 0;
+        MBBInfos[MBB].NumVALUInstsAtStart = 0;
         MaxNumVALUInstsInMiddle = 0;
----------------
Move &Info down here to avoid 2 map lookups?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:150-152
       SuccsMayReachVMEMLoad |= MBBInfos[Succ].MayReachVMEMLoad;
       NumFollowingVALUInsts =
           std::max(NumFollowingVALUInsts, MBBInfos[Succ].NumVALUInstsAtStart);
----------------
kosarev wrote:
> arsenm wrote:
> > kosarev wrote:
> > > These potentially invalidate `Info`.
> > 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?
> 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`.
Should this not ignore loops?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:154
     }
     if (AtStart)
+      MBBInfos[MBB].NumVALUInstsAtStart += NumFollowingVALUInsts;
----------------
Ditto?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134726