This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Use module flag to get code object version at IR level folow-up
ClosedPublic

Authored by cfang on Feb 3 2023, 1:54 PM.

Details

Summary

This is part of the leftover work for https://reviews.llvm.org/D143138.

In this work, we pass code object version as an argument to initialize target ID.

Diff Detail

Event Timeline

cfang created this revision.Feb 3 2023, 1:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 1:54 PM
cfang requested review of this revision.Feb 3 2023, 1:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 1:54 PM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Feb 3 2023, 3:06 PM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
807

Going from enum names to magic constants is a regression

cfang added inline comments.Feb 3 2023, 3:24 PM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
807

But now we actually switch on CodeObjectVersion which is an unsigned integer, so we have to case on integers like 2, 3, 4
Previous we switch on getHsaAbiVersion, which is an enum value.

In short, I am thinking we can no longer case on enum values now.

Do we still need getHsaAbiVersion() and the ELFABIVERSION_AMDGPU_HSA_* constants?

llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
335

Is this an accidental change?

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.h
114–115

COV looks a bit cryptic here. What if CodeObjectVersion?

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

We probably want this initialised -- regardless of whether it is then assigned a value?

cfang marked 3 inline comments as done.Feb 6 2023, 10:31 AM

Do we still need getHsaAbiVersion() and the ELFABIVERSION_AMDGPU_HSA_* constants?

In term of code object version itself, I don't think these are still needed. However, ABIVersion is used in the assembler/disassembler,
and the work to check code object version (not from command line) is on-going, so it is better to get rid of them completely in that patch.

llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
335

Right. Thanks for pointing out.

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.h
114–115

OK.

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

Initialize it to 0, and I think this value will never been used.

cfang updated this revision to Diff 495211.Feb 6 2023, 10:37 AM
cfang marked 3 inline comments as done.

Update based on reviewers' comments. Thanks.

Looks good to me, but I don't know enough about this code to approve.

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

Sorry, it seems I missed it that we also initialise it in the constructor. We probably don't need both the initialisers.

cfang added inline comments.Feb 7 2023, 1:57 PM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
807

But now we actually switch on CodeObjectVersion which is an unsigned integer, so we have to case on integers like 2, 3, 4
Previous we switch on getHsaAbiVersion, which is an enum value.

In short, I am thinking we can no longer case on enum values now.

Hi, above is my explanation of using the numbers 2, 3, 4, 5 for the cases the code object version cases. Please advice we should we do here if this is not appropriate? re-define CodeObjectVersion to a different type? Thanks.

arsenm added inline comments.Feb 8 2023, 5:16 AM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
807

I don’t understand why removed the enum cases or need to fix up the values , at worst you need a cast. It would be better to use an enum for it everywhere, it would just be a larger cosmetic change

cfang updated this revision to Diff 496247.Feb 9 2023, 2:56 PM

define an enum type for code object version number.

arsenm accepted this revision.Feb 9 2023, 4:50 PM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
748

I see now the ELF ABI version is actually off by one, so it's a different thing. Doesn't really matter

This revision is now accepted and ready to land.Feb 9 2023, 4:50 PM