Page MenuHomePhabricator

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

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

Unit TestsFailed

TimeTest
60,070 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE="/usr/bin/python3.9"
60,070 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
60,070 msx64 debian > libFuzzer.libFuzzer::minimize_crash.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest
60,090 msx64 debian > libFuzzer.libFuzzer::out-of-process-fuzz.test
Script: -- : 'RUN: at line 2'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp && mkdir /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp
60,120 msx64 debian > libFuzzer.libFuzzer::value-profile-load.test
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LoadTest.cpp -fsanitize-coverage=trace-gep -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/value-profile-load.test.tmp-LoadTest

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
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 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
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 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