This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] For OS type AMDPAL, fixed scratch on compute shader
ClosedPublic

Authored by tpr on Mar 14 2018, 7:24 AM.

Details

Summary

For OS type AMDPAL, the scratch descriptor is loaded from offset 0 of
the GIT, whose 32 bit pointer is in s0 (s8 for gfx9 merged shaders).

This commit fixes that to use offset 0x10 instead of offset 0 for a
compute shader, per the PAL ABI spec.

Change-Id: I93dffa647758e37f613bb5e0dfca840d82e6d26f

Diff Detail

Event Timeline

tpr created this revision.Mar 14 2018, 7:24 AM
arsenm accepted this revision.Mar 14 2018, 7:33 AM

LGTM

This revision is now accepted and ready to land.Mar 14 2018, 7:33 AM
This revision was automatically updated to reflect the committed changes.
tpr reopened this revision.Mar 28 2018, 7:28 AM

I had to revert this because it caused a test failure only with LLVM_ENABLE_EXPENSIVE_CHECKS.

This revision is now accepted and ready to land.Mar 28 2018, 7:28 AM
tpr updated this revision to Diff 140073.Mar 28 2018, 7:37 AM

V2: Ensure livein set for s0 (s8 in gfx9 merged shader) when using it to load
scratch decriptor.

tpr added a comment.Mar 28 2018, 8:05 AM

What's the protocol for a fix to a change that I had to revert due to test failure? Does it need to come back to review here? Or should I just have landed it?

LGTM.

In D44468#1050348, @tpr wrote:

What's the protocol for a fix to a change that I had to revert due to test failure? Does it need to come back to review here? Or should I just have landed it?

I don't believe there are completely hard rules. Personally, I'd say it depends on (a) how invasive the fix is and (b) how quickly you find it. In this particular case, I'd say the change is small enough that you could have just landed it. I'd go further and say that if the problem is so obvious that you realize it within a very short time of receiving the failure notifications, you might just commit the fix without reverting. Generally you should run the lit tests yourself locally though, to reduce the chances of this happening in the first place.

This revision was automatically updated to reflect the committed changes.