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
