[PATCH] D146023: [AMDGPU] Remove Code Object V2

[Scott Linder via Phabricator via cfe-commits]

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