This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/SI: Use a better method for determining the largest pressure sets
ClosedPublic

Authored by tstellarAMD on Aug 18 2016, 11:50 AM.

Details

Summary

There are a few different sgpr pressure sets, but we only care about
the one which covers all of the sgprs. We were using hard-coded
register pressure set names to determine the reg set id for the
biggest sgpr set. However, we were using the wrong name, and this
method is pretty fragile, since the reg pressure set names may
change.

The new method just looks for the pressure set that contains the most
reg units and sets that set as our SGPR pressure set. We've also
adopted the same technique for determining our VGPR pressure set.

Diff Detail

Event Timeline

tstellarAMD retitled this revision from to AMDGPU/SI: Use a better method for determining the largest pressure sets.
tstellarAMD updated this object.
tstellarAMD added a reviewer: arsenm.
tstellarAMD added a subscriber: llvm-commits.
arsenm added inline comments.Aug 18 2016, 12:09 PM
lib/Target/AMDGPU/SIRegisterInfo.cpp
100

DenseMap?

117

Leftevor debug printing

979

Can use the constructor parameters instead of make_pair, but this is also the default

985–989

This is the same as isSGPRSet == isVGPRSet

1004

Ditto

1006

Save size

1010–1015

This is the same as isSGPRSet == isVGPRSet

Address review comments, and remove code that was meant for a different patch.

tstellarAMD marked an inline comment as done.Aug 19 2016, 10:50 AM
tstellarAMD added inline comments.
lib/Target/AMDGPU/SIRegisterInfo.cpp
100

I used a vector instead.

arsenm accepted this revision.Aug 19 2016, 12:58 PM
arsenm edited edge metadata.

LGTM

lib/Target/AMDGPU/SIRegisterInfo.cpp
111

Preincrement

lib/Target/AMDGPU/SIRegisterInfo.h
190–195

Why do these need to check that it's not in the other set? Are there still issues from VS_32 being allocatable?

This revision is now accepted and ready to land.Aug 19 2016, 12:58 PM
lib/Target/AMDGPU/SIRegisterInfo.h
190–195

Yes, we need to make sure we ignore VS_32.

arsenm added inline comments.Aug 19 2016, 1:23 PM
lib/Target/AMDGPU/SIRegisterInfo.h
190–195

But you should never see a VS_32 virtual register. Will this be fixed when making it unallocatable? This probably needs a comment/fixme

lib/Target/AMDGPU/SIRegisterInfo.h
190–195

It's not about virtual registers, the VS_32 register pressure set contains all sgprs and vgprs, so the pressure set is affected by pretty much every register operand. I think making it unallocatable will fix this, but we probably still want an assert or something to making register sets with sgprs and vgprs is never created.

This revision was automatically updated to reflect the committed changes.