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

[cfang]

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.

[arsenm]

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

807

Going from enum names to magic constants is a regression

[cfang]

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.

[kosarev]

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

115–116

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]

In D143293#4106251, @kosarev wrote:

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

115–116

OK.

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

123

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

[cfang]

Update based on reviewers' comments. Thanks.

[kosarev]

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]

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]

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]

define an enum type for code object version number.

[arsenm]

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