[PATCH] D146023: [AMDGPU] Remove Code Object V2
Pierre-vh abandoned this revision. Pierre-vh added a comment. https://github.com/llvm/llvm-project/pull/65715 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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
Pierre-vh added a comment. ping 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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
Pierre-vh added inline comments.
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:5598
return ParseDirectiveHSAMetadata();
} else {
-if (IDVal == ".hsa_code_object_version")
cfang wrote:
> Are you sure Non-HSA does not have the four directives you deleted?
I'm really not sure, I saw `hsa` in the name and I thought it only applied to
HSA, but some tests are failing.
I'll leave them in until someone can answer for sure.
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:46
enum {
- AMDHSA_COV2 = 2,
AMDHSA_COV3 = 3,
cfang wrote:
> Should we keep this field, and just mention "unsupported"?
I'm not sure about that, I assume that we're removing as many traces of V2 as
possible from the backend. No point in keeping an unused enum entry IMO, but
I'm okay with keeping it if there's a good reason to
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:59
/// false otherwise.
bool isHsaAbiVersion3(const MCSubtargetInfo *STI);
/// \returns True if HSA OS ABI Version identification is 4,
cfang wrote:
> Are all these "isHsaAbiVersionX" no longer needed?
Yes they're needed because they implicitly check for HSA as well
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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
cfang added a comment.
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:5598
return ParseDirectiveHSAMetadata();
} else {
-if (IDVal == ".hsa_code_object_version")
Are you sure Non-HSA does not have the four directives you deleted?
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:122
std::optional getHsaAbiVersion(const MCSubtargetInfo *STI) {
if (STI && STI->getTargetTriple().getOS() != Triple::AMDHSA)
return std::nullopt;
It is fine now. But I think STI could never be null.
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:46
enum {
- AMDHSA_COV2 = 2,
AMDHSA_COV3 = 3,
Should we keep this field, and just mention "unsupported"?
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:59
/// false otherwise.
bool isHsaAbiVersion3(const MCSubtargetInfo *STI);
/// \returns True if HSA OS ABI Version identification is 4,
Are all these "isHsaAbiVersionX" no longer needed?
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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
Pierre-vh added inline comments. Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:5583 + // FIXME: Shouldn't be needed anymore? Should we remove this directive + // entirely? See `amdpal-elf.ll` - the output ASM contains both amdgcn_target This is an issue with `amdpal-elf.ll`, the run line with `llvm-mc` fails because it can't parse `amd_amdgpu_isa`. Not sure how to fix this. Should we be able to read that directive, or should we just never emit it? Some tests use it, and stg also emits it in the same test, so it's not new. It's just no longer parse-able after `isHsaAbiVersion3AndAbove` was removed - I suspect that function returned false for non-HSA OSes and was mistakenly used here to check for `!= AMDHSA`? 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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
Pierre-vh 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)
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
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 {
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
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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
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)
I don't follow this change; was the assert just incorrect previously?
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:
> 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?
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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
Pierre-vh added a comment. ping 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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
Pierre-vh removed a reviewer: jdoerfert. Pierre-vh added a comment. Herald added a reviewer: jdoerfert. ping 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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
Pierre-vh added inline comments.
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 {
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
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5437-5440
+// FIXME: Metadata Verifier doesn't work with AMDPAL MD.
+// This is a ugly workaround to avoid the verifier.
+if (MsgPackString.find("amdpal.") == StringRef::npos) {
+ AMDGPU::HSAMD::V3::MetadataVerifier Verifier(true);
arsenm wrote:
> This looks like a separate change?
Moved to D146119
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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
arsenm added a comment.
Is clover still relying on the cov2 support?
Comment at: clang/include/clang/Basic/TargetOptions.h:85
COV_None,
-COV_2 = 200,
COV_3 = 300,
I wouldn't remove the enum field, just add a comment that emission is
unsupported or something
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 {
I thought we were still going to be able to read old objects
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5437-5440
+// FIXME: Metadata Verifier doesn't work with AMDPAL MD.
+// This is a ugly workaround to avoid the verifier.
+if (MsgPackString.find("amdpal.") == StringRef::npos) {
+ AMDGPU::HSAMD::V3::MetadataVerifier Verifier(true);
This looks like a separate change?
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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
yaxunl added a comment. LGTM on clang side. 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
[PATCH] D146023: [AMDGPU] Remove Code Object V2
kzhuravl added inline comments.
Comment at: llvm/docs/AMDGPUUsage.rst:1379
.. warning::
- Code object V2 is not the default code object version emitted by
- this version of LLVM.
+ Code object V2 emission is no longer supported by this version of LLVM.
emission->generation?
Comment at: llvm/docs/AMDGPUUsage.rst:2647
.. warning::
- Code object V2 is not the default code object version emitted by this version
- of LLVM.
+ Code object V2 emission is no longer supported by this version of LLVM.
emission->generation?
Comment at: llvm/docs/AMDGPUUsage.rst:14597
.. warning::
- Code object V2 is not the default code object version emitted by
- this version of LLVM.
+ Code object V2 emission is no longer supported by this version of LLVM.
ditto
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:749-750
-case AMDGPU::AMDHSA_COV2:
- // Code object V2 only supported specific processors and had fixed
- // settings for the XNACK.
- if (Processor == "gfx600") {
Nice to see this finally go
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
