This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Don't handle export done when unify exit nodes
ClosedPublic

Authored by ruiling on Jul 7 2021, 11:36 PM.

Details

Summary

This patch aims to revert the changes introduced by D70781 D71192 D76364

D70781 was introduced to fix hardware hang where we do not insert exp-
null-done for a kill inside infinit loop. At that time we have not added
exp-null-done for kill early termination, but I believe as for now, we will
always add the exp-null-done for early termination case in LaterBranchLowering.

D71192 was introduced to handle the only_kill case, which is also been
handled by the kill early termination work.

D76364 was used to fix a regression by D71192, where we cleared the done
bit of the export in the existing program and not let the normal return
block branching to the new unified return block.

With this change, we just trust frontends have setup exp-done correctly
which is true for all existing frontends. The backend only inserts
exp-null-done for the kill cases which is handled in SILateBranchLowering.cpp.

Diff Detail

Event Timeline

ruiling created this revision.Jul 7 2021, 11:36 PM
ruiling requested review of this revision.Jul 7 2021, 11:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 11:36 PM

This is an alternative to D102830 ?

From my own recent work, I think this should be fine.
The only cases where a null export are "required" occur in control flow following an infinite loop, which obviously is never executed.

foad added a subscriber: piotr.Jul 8 2021, 2:08 AM

Adding @piotr (author of D76364) for awareness.

ruiling added a comment.EditedJul 8 2021, 3:20 AM

This is an alternative to D102830 ?

Yes, as I find that the frontend has been pretty aware of this hardware requirement and they have done lots of work when to insert the dummy export. So I think it is better we just let the frontend be responsible for such work. The backend is only responsible for the inserting the exp-done for early-termination-of-kill This would make the pass easy to maintain.

critson accepted this revision.Jul 12 2021, 7:45 PM

LGTM

Please confirm it passes the usual graphics tests.

This revision is now accepted and ready to land.Jul 12 2021, 7:45 PM

LGTM

Please confirm it passes the usual graphics tests.

I run the cts tests that are known to test this, all passed.

This revision was landed with ongoing or failed builds.Jul 14 2021, 12:07 AM
This revision was automatically updated to reflect the committed changes.