This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Update AMDHSA code object version handling
ClosedPublic

Authored by kzhuravl on Oct 8 2020, 3:27 PM.

Details

Summary
  • Define AMDHSA's code object versions in ELF.h
  • Introduce --amdhsa-code-object-version=<num> option, where <num> can be 2, 3
  • Remove code-object-v3 subtarget feature
  • Mark -m[no-]code-object-v3 options as deprecated and substitute deprecated options with new ones

Diff Detail

Event Timeline

kzhuravl created this revision.Oct 8 2020, 3:27 PM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
kzhuravl requested review of this revision.Oct 8 2020, 3:27 PM
scott.linder added inline comments.Oct 8 2020, 5:00 PM
clang/docs/ClangCommandLineReference.rst
179

There seem to be a lot of unrelated changes here; does the patch need to be rebased? Or are you just cleaning up the whole file?

If the latter could these be in a separate diff?

t-tye accepted this revision.Oct 8 2020, 5:29 PM
t-tye added inline comments.
clang/docs/ClangCommandLineReference.rst
179

Many changes in this file are the result of earlier changes in the options.td file not updating this file with the build result. So suggest submitting a review for updating this file without the code version changes first. Then the changes to this file in this review will just be code object version related.

Otherwise I think all the changes in this review are related to cleaning up the handling of the existing V2 and V3 code object formats in preparation for submitting the V4 code object changes.

llvm/docs/AMDGPUUsage.rst
910–911

Were there also cleanup in the ELF section that describes the e_flags and defines the usage of the new --amdhsa-code-object-version option?

This revision is now accepted and ready to land.Oct 8 2020, 5:29 PM
t-tye requested changes to this revision.Oct 8 2020, 5:30 PM

LGTM except the two suggestions.

This revision now requires changes to proceed.Oct 8 2020, 5:30 PM
kzhuravl marked 3 inline comments as done.Oct 8 2020, 5:38 PM
kzhuravl added inline comments.
clang/docs/ClangCommandLineReference.rst
179

Re-generating the whole file, as I think it might have been stale.

llvm/docs/AMDGPUUsage.rst
910–911

I think doc updates will come in a separate patch as we discussed earlier today.

kzhuravl updated this revision to Diff 297107.Oct 8 2020, 7:08 PM
kzhuravl marked an inline comment as done.

Address review feedback:

  • Regenerate docs in separate change
t-tye accepted this revision.Oct 8 2020, 7:25 PM

LGTM Requested changes will happen in a separate patch.

This revision is now accepted and ready to land.Oct 8 2020, 7:25 PM
MaskRay added inline comments.Oct 8 2020, 7:39 PM
clang/include/clang/Driver/Options.td
2464–2468

Does this still need m_amdgpu_Features_Group?

t-tye added inline comments.Oct 8 2020, 7:53 PM
clang/include/clang/Driver/Options.td
2464–2469

Would it be worth stating that the replacement option is "-mllvm --amdhsa-code-object-version=2" and "-mllvm --amdhsa-code-object-version=3" respectively?

kzhuravl marked 2 inline comments as done.Oct 8 2020, 8:00 PM
kzhuravl added inline comments.
clang/include/clang/Driver/Options.td
2464–2468

I changed it to m_Group on purpose. This way -target-feature +/-code-object-v3 is not added to cc1, which results in a warning:

'+/-code-object-v3' is not a recognized feature for this target (ignoring feature)

Since we removed this feature from our be.

2464–2469

Replacement option is already stated in the deprecated warning, in case this option is used. Do we really want to put the same suggestion here? I quickly glanced through ClangCommandLineReference.rst and did not see similar cases.

This revision was automatically updated to reflect the committed changes.
kzhuravl marked 2 inline comments as done.