scott.linder added inline comments.
================ Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:520 - assert(Func.getCallingConv() == CallingConv::AMDGPU_KERNEL || - Func.getCallingConv() == CallingConv::SPIR_KERNEL); + if (Func.getCallingConv() != CallingConv::AMDGPU_KERNEL && + Func.getCallingConv() != CallingConv::SPIR_KERNEL) ---------------- Pierre-vh wrote: > scott.linder wrote: > > I don't follow this change; was the assert just incorrect previously? > It's for `CodeGen/AMDGPU/no-hsa-graphics-shaders.ll`, it crashes otherwise. > > An alternative can be to change this: > ``` > if (STM.isAmdHsaOS()) > HSAMetadataStream->emitKernel(*MF, CurrentProgramInfo); > ``` > So it checks the CC and doesn't call the function if the CC is incorrect. I > don't mind either solution OK, makes sense. I don't have a particularly strong preference, but whatever the solution is I would prefer it be split into another patch. It isn't actually a result of dropping v2 support, as the bug is present in HEAD as-is. ================ Comment at: llvm/test/MC/AMDGPU/hsa-gfx10.s:3 -// RUN: llvm-mc -filetype=obj -triple amdgcn--amdhsa -mcpu=gfx1010 --amdhsa-code-object-version=2 -mattr=-wavefrontsize32,+wavefrontsize64 -show-encoding %s | llvm-readobj -S --sd --syms - | FileCheck %s --check-prefix=ELF - -// ELF: Section { ---------------- Pierre-vh wrote: > scott.linder wrote: > > Pierre-vh wrote: > > > arsenm wrote: > > > > I thought we were still going to be able to read old objects > > > I think llvm-readobj uses all of the MC/Target infrastructure so if we > > > remove emission, we also remove reading, no? > > > > > > I'm actually not sure if we plan to let readobj/readelf read COV2 object > > > files, it's an interesting question > > I think this is my biggest concern. Do we incur a huge maintenance burden > > that warrants dropping read support? How much code do we really need to > > maintain to keep the readobj/objdump like tools universal? > > > > @t-tye do you have any thoughts on whether we should maintain backwards > > compatibility in the LLVM tooling, even if we drop generation support? > It's been a while since I wrote this but IIRC there was a discussion about it > and it was fine to remove read support. An alternative may be to still > identify code object V2, but not read the metadata and instead print a > warning about the file format being deprecated? > > Or I think it's YAML, maybe we can just raw dump the MD and print a warning? > > Most of the maintenance cost would be in the MD mapper which is almost 500 > lines of code that'd just be there for the sake of "maybe some needs to read > MD" > > If we go with one of the above suggestions I can just add a test using > yml2obj that emits a V2 file Ah, alright; apologies if I was part of the discussions and am still re-litigating now :) I don't have particularly strong feelings, especially if there is some consensus that dropping is OK. Or if it isn't too onerous the "best effort" support of dumping things as-is for the old versions sounds good to me! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146023/new/ https://reviews.llvm.org/D146023 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits