Page MenuHomePhabricator

[AMDGPU] Make the uses_dynamic_stack field in the kernel descriptor and the metadata map specific to code object v5 and later
ClosedPublic

Authored by abinavpp on Sep 27 2022, 1:02 AM.

Details

Summary

Unfortunately, we have a broken handling of this in the runtime of rocm
5.3. The runtime is expected to handle this correctly when v5 becomes
the default.

Diff Detail

Event Timeline

abinavpp created this revision.Sep 27 2022, 1:02 AM
Herald added a project: Restricted Project. · View Herald Transcript
abinavpp requested review of this revision.Sep 27 2022, 1:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 1:02 AM

What's broken in the runtime? If it didn't read it before, how can this break something?

llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
420–422

Swap order of checks

abinavpp updated this revision to Diff 463422.Sep 27 2022, 10:40 PM

Address review comment

abinavpp marked an inline comment as done.Sep 27 2022, 10:40 PM
arsenm added inline comments.Oct 10 2022, 11:00 AM
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
877–879

This isn't querying the module for the code object version; probably should have that in the asm printer doInitialization

abinavpp added inline comments.Oct 10 2022, 11:46 AM
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
877–879

I didn't get you. AMDGPUAsmPrinter's ctor checks for the code-obj-ver using isHsaAbiVersion*, which checks for the AmdhsaCodeObjectVersion like this. Or should we override emitKernel() for MetadataStreamerMsgPackV5 and set this entry there. I don't think I understood the problem that we're trying to fixing by overriding AsmPrinter's doInitialization().

arsenm added a subscriber: cfang.Oct 10 2022, 11:54 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
877–879

@cfang had a patch to switch to querying the code object version from the IR module instead of the global command line flag

nhaehnle removed a subscriber: nhaehnle.Oct 11 2022, 2:05 AM
abinavpp added inline comments.Oct 11 2022, 9:57 AM
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
877–879

I think we should do this in a separate patch and for now, stick to getAmdhsaCodeObjectVersion() in this patch for consistency. I believe we should replace all getAmdhsaCodeObjectVersion() queries with the "amdgpu_code_object_version" module flag query. I would like to see that metadata map to something like the enum CodeObjectVersionKind in clang's include/clang/Basic/TargetOptions.h in llvm. This will be a big change I guess. I couldn't find @cfang 's patch for this, I'm assuming he's working on it or is done with it.

arsenm accepted this revision.Oct 11 2022, 9:59 AM
This revision is now accepted and ready to land.Oct 11 2022, 9:59 AM
This revision was landed with ongoing or failed builds.Oct 11 2022, 10:59 AM
This revision was automatically updated to reflect the committed changes.

Oops, I think I broke docs-llvm-html: https://lab.llvm.org/buildbot/#/builders/30/builds/27004. Sorry, I don't think I enabled this in my cmake at the moment. Will this fix the error:

diff --git a/llvm/docs/AMDGPUUsage.rst b/llvm/docs/AMDGPUUsage.rst
index da8a5cf19dbc..eac0883c4949 100644
--- a/llvm/docs/AMDGPUUsage.rst
+++ b/llvm/docs/AMDGPUUsage.rst
@@ -3577,6 +3577,7 @@ Code object V5 metadata is the same as
      ".uses_dynamic_stack" boolean                  Indicates if the generated machine code
                                                     is using a dynamically sized stack.
      ===================== ============= ========== =======================================
+
 ..
 
   .. table:: AMDHSA Code Object V5 Kernel Argument Metadata Map Additions and Changes

Checked by running sphinx manually. Fix: bb4abb1a005c11add7f3206faf0f526b2e54e6f8