This is an archive of the discontinued LLVM Phabricator instance.

Update AMDGPU PAL usage documentation
AcceptedPublic

Authored by cohughes on Dec 11 2020, 9:08 AM.

Details

Reviewers
tpr
foad
t-tye

Diff Detail

Event Timeline

cohughes created this revision.Dec 11 2020, 9:08 AM
cohughes requested review of this revision.Dec 11 2020, 9:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2020, 9:08 AM

I am looking to update the AMDGPU PAL documentation I was not sure who I need to add as a reviewer. I know you are familiar with the process.

cohughes updated this revision to Diff 311258.Dec 11 2020, 9:32 AM

Italicize phrase for continuity

Thanks for expanding on this documentation.

llvm/docs/AMDGPUUsage.rst
8528–8541

Why did you remove the documentation of this table?

cohughes added inline comments.Dec 14 2020, 1:25 PM
llvm/docs/AMDGPUUsage.rst
8528–8541

The existing documentation was out of date / partially wrong. Updating it would require releasing info about stuff we're not sure we can make publicly available yet. So we figured that omitting it is better than leaving the wrong stuff in there

cohughes updated this revision to Diff 312579.Dec 17 2020, 12:01 PM

Gerrit reivew changes

t-tye added inline comments.Dec 17 2020, 12:13 PM
llvm/docs/AMDGPUUsage.rst
8588–8598

Use same table syntax as other tables.

t-tye edited reviewers, added: t-tye; removed: tony-tye.Dec 17 2020, 12:14 PM
t-tye accepted this revision.Dec 17 2020, 1:11 PM

LGTM

llvm/docs/AMDGPUUsage.rst
8588–8598

Unfortunately simple tables do not allow the first column to have multiple line content so have to use a grid table.

This revision is now accepted and ready to land.Dec 17 2020, 1:11 PM
nhaehnle added inline comments.Dec 21 2020, 12:24 PM
llvm/docs/AMDGPUUsage.rst
8528–8541

You should at least bring it uptodate with what's released open-source in PAL on GitHub...

cohughes updated this revision to Diff 333980.Mar 29 2021, 2:10 PM

Minor change to metadata, and a clarification about USER_DATA0/1

cohughes updated this revision to Diff 333990.Mar 29 2021, 2:51 PM

Switch to main branch

cohughes updated this revision to Diff 333991.Mar 29 2021, 2:56 PM

Redo for diff: Minor metadata change, and clarification of USER_DATA_0/1 restriction.

t-tye added inline comments.Mar 29 2021, 3:09 PM
llvm/docs/AMDGPUUsage.rst
8573–8579

Is USER_DATA_0 and USER_DATA_1 defined somewhere?

cohughes added inline comments.Mar 29 2021, 3:17 PM
llvm/docs/AMDGPUUsage.rst
8573–8579

Specifically, it refers to SPI_SHADER_USER_DATA_*_0 (based on the shader stage) and COMPUTE_USER_DATA_0.

cohughes added inline comments.Mar 29 2021, 3:22 PM
llvm/docs/AMDGPUUsage.rst
8573–8579

Also, we refer to "any USER_DATA" register right above this with the same kind of generalization. Not sure if its worth being super specific or not.

Chiming in: they specifically refer to mmCOMPUTE_USER_DATA_0 and mmSPI_SHADER_USER_DATA_{STAGE}_0, which are in the public chip register offsets (not anywhere in LLVM right now, but present in PAL, Mesa, and any public chip register spec). I don't think we should explicitly define those values here, but using those names might make it a bit more specific and easier to find.

t-tye added inline comments.Mar 29 2021, 3:27 PM
llvm/docs/AMDGPUUsage.rst
8573–8579

In the text above it refers to "user data registers" but here it says "USER_DATA_1". So I suggest being consistent. If USER_DATA_1 is not a identifier defined somewhere else then do not use it here, use the "user data register 1" terminology used in the proceeding paragraphs.

cohughes added inline comments.Mar 29 2021, 3:30 PM
llvm/docs/AMDGPUUsage.rst
8607–8608

These are the lines I was referring to a similar generalization.

cohughes added inline comments.Mar 29 2021, 3:34 PM
llvm/docs/AMDGPUUsage.rst
8573–8579

Actually, I think I like your suggestion. I'll update it.

cohughes updated this revision to Diff 334004.Mar 29 2021, 3:44 PM

Code review changes

t-tye accepted this revision.Mar 29 2021, 7:07 PM

LGTM

foad added a comment.Mar 30 2021, 2:10 AM

Please make the commit message a bit more descriptive than just "update".

cohughes updated this revision to Diff 334119.Mar 30 2021, 5:36 AM

Update commit message

tpr added a comment.Mar 31 2021, 12:12 AM

Please make the commit message a bit more descriptive than just "update".

Sorry, I landed this on Colin's request before seeing this comment. Also I forgot to add the review number to the commit message.