[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam added a comment. In D93525#2952012 , @JonChesterfield wrote: > This is mentioned as broken in the referenced patch and landed with @t-tye > still marked as requires changes. Revert warranted? > > Testing looks very sparse for something handling

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

JonChesterfield added a comment. This is mentioned as broken in the referenced patch and landed with @t-tye still marked as requires changes. Revert warranted? Testing looks very sparse for something handling archives. Presumably it has the same set of bugs as Repository: rG LLVM Github

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam added a comment. @rsmith, Thanks for pointing out this issue. I have proposed a fix in D105285 . Please review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93525/new/ https://reviews.llvm.org/D93525

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

rsmith added inline comments. Comment at: clang/test/Driver/clang-offload-bundler.c:390 +// RUN: clang-offload-bundler -unbundle -type=a -targets=openmp-amdgcn-amd-amdhsa--gfx906,openmp-amdgcn-amd-amdhsa--gfx908 -inputs=%t.input-archive.a

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

grokos added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:147 +Target.split(Components, '-', 5); +Components.resize(6); +this->OffloadKind = Components[0]; saiislam wrote: > grokos wrote: > > Leftover?

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam added inline comments. Comment at: clang/docs/ClangOffloadBundler.rst:128 + + --- + grokos wrote: > A bit of wordplay, but it's weird that a *triple* now has 4 elements... I think llvm::Triple it is named Triple because of historical reasons.

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

yaxunl added a comment. In D93525#2849815 , @saiislam wrote: > @yaxunl > this patch on its own is failing in our internal CI. I have an internal patch > (542569) to integrate it cleanly there. Fine. Thanks. Repository: rG LLVM Github Monorepo

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

grokos added inline comments. Comment at: clang/docs/ClangOffloadBundler.rst:128 + + --- + A bit of wordplay, but it's weird that a *triple* now has 4 elements... Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:147 +

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam added a comment. @yaxunl this patch on its own is failing in our internal CI. I have an internal patch (542569) to integrate it cleanly there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93525/new/ https://reviews.llvm.org/D93525

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf7ce532d622d: [clang-offload-bundler] Add unbundling of archives

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

jdoerfert accepted this revision. jdoerfert added a comment. Looks reasonable to me. We can always refine it as we go. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:137 +/// * GPUArch (Optional) - Processor name, like gfx906 or sm_30 +/// In presence

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam added a comment. In D93525#2838535 , @yaxunl wrote: > LGTM. Thanks! Pls make sure it passes internal CI (ePSDB) before committing. Sure, I will take care of it. Thanks! Any comments @jdoerfert ? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

yaxunl accepted this revision. yaxunl added a comment. LGTM. Thanks! Pls make sure it passes internal CI (ePSDB) before committing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93525/new/ https://reviews.llvm.org/D93525

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam updated this revision to Diff 354197. saiislam added a comment. Generalized padding of Triple fields of Bundle Entry ID while generating command for clang-offload-bundler. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93525/new/

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7619 if (CurKind == Action::OFK_HIP && CurDep->getOffloadingArch()) { - Triples += '-'; + Triples += "--"; Triples += CurDep->getOffloadingArch(); This is not

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam updated this revision to Diff 353922. saiislam added a comment. Updated clang and hip tests to ensure that all 4 components of triple are mandataroly available in the bundle entry ID. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

yaxunl added a comment. Does this pass internal CI (ePSDB) ? I am concerned about the enforcement of the canonical format of target triple since this may break backward compatibility. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93525/new/

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93525/new/ https://reviews.llvm.org/D93525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam updated this revision to Diff 350897. saiislam added a comment. Removed Triple format example from documentation and simplified conditional calling of bundling/unbundling functions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93525/new/

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

jdoerfert added a comment. Thanks for splitting this. I quickly went over it only. Comment at: clang/docs/ClangOffloadBundler.rst:137 + + myarch--myOS- not needed here. Comment at:

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam updated this revision to Diff 350884. saiislam added a comment. Removed unused header. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93525/new/ https://reviews.llvm.org/D93525 Files: clang/docs/ClangOffloadBundler.rst

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

saiislam updated this revision to Diff 350883. saiislam added a comment. 1. Removed TargetID support, to be reviewed in a followup patch. 2. Added OffloadTargetInfo class to encapsulate handling of bundle entry ID components: OffloadKind, Triple, GPUArch. Repository: rG LLVM Github Monorepo

[PATCH] D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives

jdoerfert added a comment. Can we split this patch now and make progress? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93525/new/ https://reviews.llvm.org/D93525 ___ cfe-commits mailing list