Page MenuHomePhabricator

[AMDGPU] Increased the number of implicit argument bytes for both OpenCL and HIP (CLANG).
ClosedPublic

Authored by cdevadas on Tue, Jun 25, 1:00 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

cdevadas created this revision.Tue, Jun 25, 1:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jun 25, 1:00 AM

can you try compile an empty HIP kernel and see what metadata is generated by backend?

If I remember correctly, backend generates kernel arg metadata based on the number of implicit kernel args. It knows how to handle 48 but I am not sure what will happen if it becomes 56.

Hi Sam,
The compiler generates metadata for the first 48 bytes. I compiled a sample code and verified it. The backend does nothing for the extra bytes now.
I will soon submit the backend patch to generate the new metadata.

tra added a subscriber: tra.Tue, Jun 25, 10:24 AM

Hi Sam,
The compiler generates metadata for the first 48 bytes. I compiled a sample code and verified it. The backend does nothing for the extra bytes now.
I will soon submit the backend patch to generate the new metadata.

Something that adds ability to handle extra args should be in place at the same time or earlier than the knob that enables the feature.
Ordering of patches matters. You don't know at which revision users will check out the tree and we do want the tree to be buildable and functional at every revision.
Even if breaking the logical order appears to be benign in this case, it would still be better to commit them in order.

I propose to wait until your upcoming patch is up for review, add it as a dependency here and land patches in the right order.

I have created the review for adding the metadata in the backend. https://reviews.llvm.org/D63886
Marking the current review depended on D63886.

The backend changes, D63886 are in the upstream now with revision rL365217.
Please review this patch.

yaxunl accepted this revision.Mon, Jul 8, 8:22 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Mon, Jul 8, 8:22 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 10, 8:10 AM