[Comgr] Add an unpackaging action #668

MixedMatched · 2025-11-24T16:58:14Z

Currently, clang uses the clang-offload-bundler to offload GPU binaries, but the llvm-offload-binary packager is a possible replacement for GPU offloading. This PR adds support to Comgr for retrieving the files from an llvm-offload-binary package through an Unpackager class and an AMD_COMGR_ACTION_UNPACKAGE action.

github-actions · 2025-11-24T16:58:36Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

z1-cciauto · 2025-11-24T16:58:57Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/2951

amd/comgr/include/amd_comgr.h.in

amd/comgr/src/comgr-compiler.cpp

amd/comgr/test-lit/comgr-sources/unpackage.c

lamb-j · 2025-11-24T21:27:19Z

amd/comgr/src/comgr-compiler.cpp

+      case llvm::object::IMG_Object:
+        FileExtension = "o";
+        break;
+      case llvm::object::IMG_Bitcode:
+        FileExtension = "bc";
+        break;
+      case llvm::object::IMG_Cubin:
+        FileExtension = "cubin";
+        break;
+      case llvm::object::IMG_Fatbinary:
+        FileExtension = "fatbin";
+        break;
+      case llvm::object::IMG_PTX:
+        FileExtension = "ptx";
+        break;
+      case llvm::object::IMG_SPIRV:
+        FileExtension = "spv";
+        break;
+      default:
+        FileExtension = "unknown";
+        break;


Should we ERROR_INVALID_ARGUMENT for unsupported ImageKinds? Cubin/PTX?

I was thinking that we should imagine ImageKind as a non-exhaustive tag. All image data types are treated the exact same way (basically directly copying the byte data into a file descriptor) and I don't imagine that will need to change at any point, since text data would be treated the same way. I thought a .unknown fallback would be more justifiable than an error because, if it's a well-formed package, it shouldn't matter what type it is. OTOH, if packages do gain some sort of differentiated images in the future, our implementation might unexpectedly break. What do you think makes more sense?

I'm mostly thinking about it for the return AMD_COMGR_DATA_KIND

But maybe instead of erroring, we could set the output file DATA_KIND to AMD_COMGR_DATA_KIND_UNDEF (for the ImageKinds that don't have a corresponding DATA_KIND)?

Though looking more, seems like DATA_KIND_UNDEF is used to signal destroyed/invalid data objects, so that probably won't work:

llvm-project/amd/comgr/src/comgr.cpp

Line 312 in 41f7de0

DataKind = AMD_COMGR_DATA_KIND_UNDEF;

Sure, that makes sense. Do you think we should simply return an error for "unsupported" filetypes or should there be a different data kind for valid but unknown files? I think my preference would probably be error for now, and then possibly add that data kind in the future.

z1-cciauto · 2025-12-12T16:52:36Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/3258

z1-cciauto · 2025-12-12T16:56:47Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/3259

z1-cciauto · 2025-12-12T17:04:42Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/3260

z1-cciauto · 2025-12-12T17:25:46Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/3262

z1-cciauto · 2025-12-12T19:33:13Z

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/3265

lamb-j · 2025-12-12T20:11:25Z

@jhuber6

jhuber6

Some initial comments, I don't know the COMGR style at all

jhuber6 · 2025-12-12T20:20:13Z

amd/comgr/src/comgr-compiler.cpp


-  std::unique_ptr<MCRegisterInfo> MRI(TheTarget->createMCRegInfo(
-      llvm::Triple(Opts.Triple)));
+  std::unique_ptr<MCRegisterInfo> MRI(


A lot of these seem like unrelated format changes

@MixedMatched we could apply these formatting changes as a separate patch, land it, and rebase this one on top of that

jhuber6 · 2025-12-12T20:20:52Z

amd/comgr/src/comgr-compiler.cpp

+    // if supplied file isn't a package, return an error
+    if (Input->DataKind != AMD_COMGR_DATA_KIND_PACKAGE) {
+      return AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT;
+    }


Suggested change

// if supplied file isn't a package, return an error

if (Input->DataKind != AMD_COMGR_DATA_KIND_PACKAGE) {

return AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT;

}

// if supplied file isn't a package, return an error.

if (Input->DataKind != AMD_COMGR_DATA_KIND_PACKAGE)

return AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT;

Does COMGR follow the LLVM coding style? Would do it like this and later if so

Where possible we try to use the same style as LLVM

jhuber6 · 2025-12-12T20:25:26Z

amd/comgr/src/comgr-compiler.cpp

+    }
+
+    // Generate random name if none provided
+    if (!strcmp(Input->Name, "")) {


This should be a StringRef that checks if it's empty, that will correctly handle nullptr.

jhuber6 · 2025-12-12T20:26:59Z

amd/comgr/src/comgr-compiler.cpp

+    // Generate random name if none provided
+    if (!strcmp(Input->Name, "")) {
+      const size_t BufSize = sizeof(char) * 30;
+      char *Buf = (char *)malloc(BufSize);


Does this ever get freed? Much better to use a unique pointer or a SmallVector than to use manual memory management.

Also, if you're just creating a random filename I would suggest using llvm::sys::fs::createUniquePath or similar.

jhuber6 · 2025-12-12T20:28:06Z

amd/comgr/src/comgr-compiler.cpp

+    StringRef OutputPrefix = Input->Name;
+    size_t Index = OutputPrefix.find_last_of(".");
+    OutputPrefix = OutputPrefix.substr(0, Index);


Use llvm::sys::path::extension if that's what you're after.

jhuber6 · 2025-12-12T20:30:53Z

amd/comgr/src/comgr-compiler.cpp

+
+      for (StringRef Entry : ActionInfo->PackageEntryIDs) {
+        // TODO: this should probably check compatability, not strict equivalence
+        if (Entry == Target) {


Could we use llvm::object::areTargetsCompatible?

jhuber6 · 2025-12-12T20:32:38Z

amd/comgr/src/comgr-compiler.cpp

+    }
+
+    auto *DataKind = DataKinds.begin();
+    for (StringRef OutputFilePath : OutputFileNames) {


Could we use llvm::zip_equal and C++17 structured bindings to make this cleaner?

jhuber6 · 2025-12-12T20:37:31Z

amd/comgr/src/comgr-unpackage-command.cpp

+amd_comgr_status_t UnpackageCommand::execute(raw_ostream &LogS) {
+  StringMap<StringRef> Worklist;
+  const auto *Output = OutputFileNames.begin();
+  for (auto &Target : TargetNames) {


Can probably zip_equal here as well

jhuber6 · 2025-12-12T20:38:01Z

amd/comgr/src/comgr-unpackage-command.cpp

+    OutputBuffer.resize_for_overwrite(OutputSize + sizeof(OneOutputFileSize) +
+                                      OneOutputFileSize);
+
+    memcpy(OutputBuffer.data() + OutputSize, &OneOutputFileSize,


Do we need raw memcpy here? Seems a little weird but I don't know the underlying format we're writing to

jhuber6 · 2025-12-12T20:39:08Z

amd/comgr/src/comgr-unpackage-command.cpp

+    const llvm::object::OffloadBinary *Binary = File.getBinary();
+    StringRef Triple = Binary->getTriple();
+    StringRef Arch = Binary->getArch();
+    std::string Target = (Triple + "-" + Arch).str();


I preferred to handle this as an ordered pair, that's what TargetID's type alias is. But I guess it's handled as a string here so we need compatibility?

lamb-j · 2025-12-12T23:39:59Z

amd/comgr/test-lit/unpackage-test.hip

+// RUN: clang -c -x hip --offload-arch=gfx900 \
+// RUN: -nogpulib -nogpuinc -emit-llvm \
+// RUN: --offload-device-only %s -o %t.gfx900.bc
+//
+// RUN: clang -c -x hip --offload-arch=gfx1030 \
+// RUN: -nogpulib -nogpuinc -emit-llvm \
+// RUN: --offload-device-only %s -o %t.gfx1030.bc
+//
+// RUN: llvm-offload-binary \
+// RUN: --image=file=%t.gfx900.bc,triple=amdgcn-amd-amdhsa,arch=gfx900,kind=hip \
+// RUN: --image=file=%t.gfx1030.bc,triple=amdgcn-amd-amdhsa,arch=gfx1030,kind=hip \
+// RUN: -o %t.package.bc
+//
+// COM: extract using Comgr
+// RUN: unpackage %t.package.bc amdgcn-amd-amdhsa-gfx900 %t.gfx900.output.bc
+// RUN: llvm-dis %t.gfx900.output.bc -o - | FileCheck --check-prefixes=BOTH,GFX9 %s


@jhuber6 is there a way to generate a package for this test via a single clang command?

Something analogous to "clang -c -emit-llvm --offload-device-only --gpu-bundle-output"?

MixedMatched and others added 5 commits November 20, 2025 12:09

[comgr] Initial package support for comgr

cc95df7

comgr-compiler build fixes

84c1443

more comgr-compiler build fixes

5e286c5

comgr-compiler build fixes 3

c545ea6

create initial test infrastructure

a090bbd

lamb-j reviewed Nov 24, 2025

View reviewed changes

amd/comgr/include/amd_comgr.h.in Outdated Show resolved Hide resolved

lamb-j reviewed Nov 24, 2025

View reviewed changes

amd/comgr/src/comgr-compiler.cpp Outdated Show resolved Hide resolved

lamb-j reviewed Nov 24, 2025

View reviewed changes

amd/comgr/test-lit/comgr-sources/unpackage.c Outdated Show resolved Hide resolved

lamb-j reviewed Nov 24, 2025

View reviewed changes

add test infrastructure and do a few small fixes

393b869

fix instances of "bundle" that should be "package"

ee584d8

fix a few more erroneous instances of "bundle"

2682844

Merge branch 'amd-staging' into comgr-package

db128dd

MixedMatched marked this pull request as ready for review December 12, 2025 17:25

add missing break in case statement

3651b15

lamb-j requested review from kzhuravl and yxsamliu December 12, 2025 20:10

jhuber6 reviewed Dec 12, 2025

View reviewed changes

lamb-j requested a review from jmmartinez December 12, 2025 23:36

lamb-j reviewed Dec 12, 2025

View reviewed changes

[Comgr] Add an unpackaging action #668

Are you sure you want to change the base?

[Comgr] Add an unpackaging action #668

Uh oh!

Conversation

MixedMatched commented Nov 24, 2025

Uh oh!

github-actions bot commented Nov 24, 2025

Uh oh!

z1-cciauto commented Nov 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

z1-cciauto commented Dec 12, 2025

Uh oh!

z1-cciauto commented Dec 12, 2025

Uh oh!

z1-cciauto commented Dec 12, 2025

Uh oh!

z1-cciauto commented Dec 12, 2025

Uh oh!

z1-cciauto commented Dec 12, 2025

Uh oh!

lamb-j commented Dec 12, 2025

Uh oh!

jhuber6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants