This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] For amdpal, widen interpolation mode workaround
ClosedPublic

Authored by tpr on Sep 12 2017, 1:00 PM.

Details

Summary

The interpolation mode workaround ensures that at least one
interpolation mode is enabled in PSInputAddr. It does not also check
PSInputEna on the basis that the user might enable bits in that
depending on run-time state.

However, for amdpal os type, the user does not enable some bits after
compilation based on run-time states; the register values being
generated here are the final ones set in the hardware. Therefore, apply
the workaround to PSInputAddr and PSInputEnable together. (The case
where a bit is set in PSInputAddr but not in PSInputEnable is where the
frontend set up an input arg for a particular interpolation mode, but
nothing uses that input arg. Really we should have an earlier pass that
removes such an arg.)

Event Timeline

tpr created this revision.Sep 12 2017, 1:00 PM
arsenm added inline comments.Sep 15 2017, 10:07 AM
test/CodeGen/AMDGPU/amdpal-psenable.ll
22

Can we rename these attributes to match the naming convention and start with an amdgpu- prefix

tpr added inline comments.Sep 16 2017, 1:26 AM
test/CodeGen/AMDGPU/amdpal-psenable.ll
22

Maybe, but that is unrelated to what this change is about. I suggest doing it in a different change, Also it would need coordination between the backend, mesa, and now the amdpal frontend.

tpr added a comment.Sep 25 2017, 12:18 AM

Ping.

(We don't want to rename those attributes in this change. This change did not add the attributes, and they are already in use by Mesa.)

tpr marked 2 inline comments as done.Sep 26 2017, 12:17 AM
nhaehnle added inline comments.Sep 29 2017, 8:28 AM
lib/Target/AMDGPU/SIISelLowering.cpp
1512–1518

Could you make this guarded by isAmdPalOS? For Mesa, it's possible that another shader part will enable some of those input bits, so we want to set those enables only after the combined enable mask is known.

tpr added inline comments.Oct 2 2017, 11:52 PM
lib/Target/AMDGPU/SIISelLowering.cpp
1512–1518

Oops, it was guarded by isAmdPalOS in my first local version of this change. But I seem to have lost it.

I will re-add the guard.

tpr updated this revision to Diff 117566.Oct 3 2017, 12:48 PM

Addressed review comment.

tpr added a comment.Oct 3 2017, 12:53 PM

Oops, I have still failed to protect it in isAmdPalOS. Back in a minute with a real fix.

tpr updated this revision to Diff 117569.Oct 3 2017, 12:56 PM

Really address review comment.

tpr marked 2 inline comments as done.Oct 3 2017, 12:56 PM
This revision is now accepted and ready to land.Oct 10 2017, 2:08 AM
This revision was automatically updated to reflect the committed changes.