This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Restrict the number of instructions to scan for getPointerDependencyFrom
AbandonedPublic

Authored by cfang on Jun 8 2020, 2:46 PM.

Details

Reviewers
arsen
rampitec
Summary

This is for getPointerDependencyFrom in AMDGPUAnnotateUniformValues.
With the default scan limit of 100 instruction per block, we can reduce the compile time
of a special kernel from longer than 2 hours to 7 minutes.

Diff Detail

Event Timeline

cfang created this revision.Jun 8 2020, 2:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 2:46 PM
arsenm added inline comments.Jun 8 2020, 3:03 PM
llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp
109

I don't see how this changes anything. This is what the default null case does?

unsigned DefaultLimit = getDefaultBlockScanLimit();
if (!Limit)
  Limit = &DefaultLimit;
cfang marked an inline comment as done.Jun 8 2020, 5:08 PM
cfang added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp
109

Ooh, the change is even not correct due to the weird design of the Limit usage:

--*Limit;
if (!*Limit)
  return MemDepResult::getUnknown();

This will usually causes trouble if you don't use default nullptr. You will have to set the Limit before each call.

To say that, the use at DeadStoreElimination.cpp is also not correct.

arsenm added inline comments.Jun 8 2020, 5:45 PM
llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp
109

Seems like you should fix that instead of working around it here, or make it a mandatory argument?

cfang marked an inline comment as done.Jun 8 2020, 8:58 PM
cfang added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp
109

We should fix DeadStoreElimination case anyway.

And it seems DeadStoreElimination is the only case using explicit argument. However I am using
an argument is still useful which allows the user to specify the limit instead of using the default.
We may just pass by value with a negative meaning default.

cfang abandoned this revision.Jul 29 2020, 10:19 AM

https://reviews.llvm.org/D84873 addresses the same issue. So this is no longer needed.