This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kosarev on Sep 27 2022, 3:41 AM.

Details

Diff Detail

Event Timeline

kosarev created this revision.Sep 27 2022, 3:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 3:41 AM
kosarev requested review of this revision.Sep 27 2022, 3:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 3:41 AM
kosarev edited the summary of this revision. (Show Details)Sep 27 2022, 3:45 AM
kosarev added reviewers: foad, arsenm, rampitec, nhaehnle.
kosarev added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp
150–152

These potentially invalidate Info.

arsenm added inline comments.Sep 27 2022, 7:04 AM
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 added inline comments.Sep 27 2022, 7:48 AM
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.

arsenm accepted this revision.Sep 30 2022, 5:38 AM

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?

This revision is now accepted and ready to land.Sep 30 2022, 5:38 AM
kosarev updated this revision to Diff 464239.Sep 30 2022, 6:17 AM

Updated as suggested.

kosarev marked 2 inline comments as done.Sep 30 2022, 6:23 AM

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.

This revision was landed with ongoing or failed builds.Sep 30 2022, 6:28 AM
This revision was automatically updated to reflect the committed changes.