Page MenuHomePhabricator

[clang][amdgpu] Use implicit code object version
ClosedPublic

Authored by JonChesterfield on Thu, Apr 22, 11:23 AM.

Details

Summary

[clang][amdgpu] Use implicit code object version

At present, clang always passes amdhsa-code-object-version on to -cc1. That is
great for certainty over what object version is being used when debugging.

Unfortunately, the command line argument is in AMDGPUBaseInfo.cpp in the amdgpu
target. If clang is used with an llvm compiled with DLLVM_TARGETS_TO_BUILD
that excludes amdgpu, this will be diagnosed (as discovered via D98658):

  • Unknown command line argument '--amdhsa-code-object-version=4'

This means that clang, built only for X86, can be used to compile the nvptx
devicertl for openmp but not the amdgpu one. That would shortly spawn fragile
logic in the devicertl cmake to try to guess whether the clang used will work.

This change omits the amdhsa-code-object-version parameter when it matches the
default that AMDGPUBaseInfo.cpp specifies, with a comment to indicate why. As
this is the only part of clang's codegen for amdgpu that depends on the target
in the back end it suffices to build the openmp runtime on most (all?) systems.

It is a non-functional change, though observable in the updated tests and when
compiling with -###. It may cause minor disruption to the amd-stg-open branch.

Revision of D98746, builds on refactor in D101077

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Thu, Apr 22, 11:23 AM
Herald added a project: Restricted Project. · View Herald Transcript

Diff created relative to D101077, not relative to main. Iiuc that will work out fine if the NFC lands first.

clang/test/Driver/hip-code-object-version.hip
55–56

This is the interesting test change - we still use 'hipv4', but no longer pass amdhsa-code-object-version=4 explicitly.

The other tests don't depend on code object version so removing the regex from those seems a strict improvement.

JonChesterfield planned changes to this revision.Thu, Apr 22, 4:52 PM

LGTM. However, I would like to hear Tony's opinion.

yaxunl accepted this revision.Fri, Apr 23, 3:04 PM

I talked with Tony about the code object version arguments of clang. In general we think we should move toward the direction of clang being agnostic about code object version and let backend decide which code object version to use. This patch is in line with the direction we are moving towards.

This revision is now accepted and ready to land.Fri, Apr 23, 3:04 PM

In general we think we should move toward the direction of clang being agnostic about code object version and let backend decide which code object version to use

This sounds the right direction to me too. If we can get all code object choices out of the front ends we'll also get consistency between the various languages.

This revision was automatically updated to reflect the committed changes.