This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Make default AMDHSA Code Object Version to be 5
ClosedPublic

Authored by cfang on Jul 14 2022, 4:29 PM.

Details

Summary

Also update LIT tests and docs.

Diff Detail

Event Timeline

cfang created this revision.Jul 14 2022, 4:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 4:29 PM
cfang requested review of this revision.Jul 14 2022, 4:29 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: MaskRay, wdng. · View Herald Transcript

Are we really ready to switch the default? We aren't even optimizing the new locations yet?

Typo in patch title

cfang retitled this revision from AMDGPU: Make default AMDHSA Code Pbject Version to be 5 to AMDGPU: Make default AMDHSA Code Object Version to be 5.Jul 14 2022, 5:18 PM
arsenm accepted this revision.Nov 16 2022, 3:39 PM

LGTM. I thought this already happened?

This revision is now accepted and ready to land.Nov 16 2022, 3:39 PM
ronlieb requested changes to this revision.Nov 16 2022, 3:46 PM
ronlieb added a subscriber: ronlieb.

lets hold off landing for a few more days?
checking some internal related testing results

Thanks,

This revision now requires changes to proceed.Nov 16 2022, 3:46 PM
lamb-j added a subscriber: lamb-j.May 9 2023, 12:04 PM
lamb-j added inline comments.
clang/test/Driver/hip-device-libs.hip
144
154

Should this be ABI5? That is, if CodeObjectVersion < 4, should it be set to 5 (instead of 4)?

May also be related to a needed change here: https://github.com/llvm/llvm-project/blob/6db007a0654ed7a6ed5c3aa3b61a937c19a6bc6b/clang/lib/Driver/ToolChains/ROCm.h#L29

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
31–34
arsenm added a comment.Jun 8 2023, 5:10 PM

ping, my understand is this landed in amd-stg-open and not upstream

cfang marked 3 inline comments as done.Jun 12 2023, 3:07 PM
cfang added inline comments.
clang/test/Driver/hip-device-libs.hip
144

Yes. Should comment as 500 in these places. Thanks.

154

No. it should be 4 if <=4. I think this is mainly for device-lib versions (400.bc vs 500.bc).

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
31–34

You are right. Done.

cfang updated this revision to Diff 530681.Jun 12 2023, 3:10 PM
cfang marked 3 inline comments as done.

Rebase and fixes LIT tests.

Is there any concerns to merge this patch to upstream?

ping, my understand is this landed in amd-stg-open and not upstream

No, it has not landed in amd-stg-open yet.
Konstantin would know the current status.

Ping. This patch needs a permission to go. Thanks.

Ping. This patch needs a permission to go. Thanks.

I don't fully grasp what the OpenMP issue was. Is the OpenMP handling done?

The openmp version handling patch is in, so can this go in now?

The openmp version handling patch is in, so can this go in now?

Yes, I have picked up this patch. Will post a revision soon.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 12 2023, 1:23 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript