This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Remove Code Object V2
AbandonedPublic

Authored by Pierre-vh on Mar 14 2023, 1:42 AM.

Details

Summary

Code Object V2 has been deprecated for more than a year now. We can safely remove it from LLVM.

  • [clang] Remove support for the -mcode-object-version=2 option.
  • [lld] Remove/refactor tests that were still using COV2
  • [llvm] Update AMDGPUUsage.rst
    • Code Object V2 docs are left for informational purposes because those code objects may still be supported by the runtime/loaders for a while.
  • [AMDGPU] Remove COV2 emission capabilities.
  • [AMDGPU] Remove MetadataStreamerYamlV2 which was only used by COV2
  • [AMDGPU] Update all tests that were still using COV2
    • They are either deleted or ported directly to code object v4 (as v3 is also planned to be removed soon).

Depends on D145671

Diff Detail

Event Timeline

Pierre-vh created this revision.Mar 14 2023, 1:42 AM
Herald added a project: Restricted Project. · View Herald Transcript
Pierre-vh requested review of this revision.Mar 14 2023, 1:42 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Pierre-vh retitled this revision from [AMDGPU] Remove Code Object V2 Code Object V2 has been deprecated for more than a year now. We can safely remove it from LLVM. to [AMDGPU] Remove Code Object V2.Mar 14 2023, 1:42 AM
Pierre-vh edited the summary of this revision. (Show Details)
Pierre-vh added reviewers: Restricted Project, arsenm, kzhuravl, foad, rampitec, yaxunl, t-tye, artem, b-sumner.
kzhuravl added inline comments.Mar 14 2023, 6:36 AM
llvm/docs/AMDGPUUsage.rst
1442

emission->generation?

2710

emission->generation?

14660

ditto

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
750–751

Nice to see this finally go

LGTM on clang side.

Is clover still relying on the cov2 support?

clang/include/clang/Basic/TargetOptions.h
85

I wouldn't remove the enum field, just add a comment that emission is unsupported or something

llvm/test/MC/AMDGPU/hsa-gfx10.s
3

I thought we were still going to be able to read old objects

llvm/tools/llvm-readobj/ELFDumper.cpp
5437–5440 ↗(On Diff #504984)

This looks like a separate change?

Pierre-vh updated this revision to Diff 505414.Mar 15 2023, 2:53 AM
Pierre-vh marked 6 inline comments as done.

Address comments, split some change in D146119

llvm/test/MC/AMDGPU/hsa-gfx10.s
3

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

llvm/tools/llvm-readobj/ELFDumper.cpp
5437–5440 ↗(On Diff #504984)

Moved to D146119

Pierre-vh edited the summary of this revision. (Show Details)May 2 2023, 11:46 PM
scott.linder added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
523–524

I don't follow this change; was the assert just incorrect previously?

llvm/test/MC/AMDGPU/hsa-gfx10.s
3

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?

Pierre-vh added inline comments.Jun 6 2023, 5:43 AM
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
523–524

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

llvm/test/MC/AMDGPU/hsa-gfx10.s
3

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

scott.linder added inline comments.Jun 6 2023, 12:04 PM
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
523–524

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.

llvm/test/MC/AMDGPU/hsa-gfx10.s
3

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!

Pierre-vh added inline comments.Aug 1 2023, 12:29 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
5583 ↗(On Diff #545938)

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?

Pierre-vh updated this revision to Diff 546714.Aug 2 2023, 11:48 PM

Fix non-HSA

cfang added a subscriber: cfang.Aug 3 2023, 12:04 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
5598 ↗(On Diff #546714)

Are you sure Non-HSA does not have the four directives you deleted?

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
105

It is fine now. But I think STI could never be null.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
47

Should we keep this field, and just mention "unsupported"?

60

Are all these "isHsaAbiVersionX" no longer needed?

Pierre-vh updated this revision to Diff 547116.Aug 4 2023, 12:14 AM
Pierre-vh marked 4 inline comments as done.

Fix tests, address comments

Pierre-vh updated this revision to Diff 547117.Aug 4 2023, 12:16 AM

Simplify ParseDirectiveHSAMetadata as it's now HSA only

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
5598 ↗(On Diff #546714)

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.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
47

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

60

Yes they're needed because they implicitly check for HSA as well