Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
Thanks for expanding on this documentation.
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11057–11070 | Why did you remove the documentation of this table? |
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11057–11070 | 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 |
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11262–11272 | Use same table syntax as other tables. |
LGTM
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11262–11272 | Unfortunately simple tables do not allow the first column to have multiple line content so have to use a grid table. |
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11057–11070 | You should at least bring it uptodate with what's released open-source in PAL on GitHub... |
Redo for diff: Minor metadata change, and clarification of USER_DATA_0/1 restriction.
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11251–11253 | Is USER_DATA_0 and USER_DATA_1 defined somewhere? |
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11251–11253 | Specifically, it refers to SPI_SHADER_USER_DATA_*_0 (based on the shader stage) and COMPUTE_USER_DATA_0. |
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11251–11253 | 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.
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11251–11253 | 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. |
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11243–11244 | These are the lines I was referring to a similar generalization. |
llvm/docs/AMDGPUUsage.rst | ||
---|---|---|
11251–11253 | Actually, I think I like your suggestion. I'll update it. |
Sorry, I landed this on Colin's request before seeing this comment. Also I forgot to add the review number to the commit message.
Why did you remove the documentation of this table?