Page MenuHomePhabricator

Emit predefined macro for wavefront size for amdgcn
ClosedPublic

Authored by yaxunl on Sep 26 2020, 7:41 PM.

Details

Summary

Also fix the issue of multiple -m[no-]wavefrontsize64 options to make the last one wins.

Diff Detail

Event Timeline

yaxunl created this revision.Sep 26 2020, 7:41 PM
yaxunl requested review of this revision.Sep 26 2020, 7:41 PM
yaxunl updated this revision to Diff 294534.Sep 26 2020, 8:34 PM

fix typo

arsenm added inline comments.Sep 28 2020, 9:36 AM
clang/lib/Basic/Targets/AMDGPU.h
421

Why is this not redundant with the features check?

clang/test/Driver/amdgpu-macros.cl
351

Macros should be all caps

yaxunl marked an inline comment as done.Sep 28 2020, 9:42 AM
yaxunl added inline comments.
clang/lib/Basic/Targets/AMDGPU.h
421

You mean the above check for target ID features? It only checks features in target ID, i.e. xnack and sramecc.

clang/test/Driver/amdgpu-macros.cl
351

will change

yaxunl updated this revision to Diff 294734.Sep 28 2020, 9:47 AM
yaxunl marked an inline comment as done.

capitalize macro

yaxunl updated this revision to Diff 294790.Sep 28 2020, 1:05 PM

revised by Matt's comments.

yaxunl marked an inline comment as done.Sep 30 2020, 4:00 AM

ping

arsenm added inline comments.Sep 30 2020, 6:22 AM
clang/test/Driver/amdgpu-macros.cl
380–381

Can you also check with the flags reversed, -mno-wavefrontsize64 -mwavefrontsize64

yaxunl updated this revision to Diff 295297.Sep 30 2020, 8:28 AM
yaxunl edited the summary of this revision. (Show Details)

Add test and fix multiple -m[no-]wavefrontsize64 issue.

arsenm added inline comments.Sep 30 2020, 8:39 AM
clang/lib/Driver/ToolChains/AMDGPU.cpp
394–395 ↗(On Diff #295297)

Why isn't this using hasFlag?
e.g. like

DriverArgs.hasFlag(options::OPT_fcuda_flush_denormals_to_zero,
                           options::OPT_fno_cuda_flush_denormals_to_zero,
                           getDefaultDenormsAreZeroForTarget(Kind)))
yaxunl added inline comments.Sep 30 2020, 8:47 AM
clang/lib/Driver/ToolChains/AMDGPU.cpp
394–395 ↗(On Diff #295297)

hasFlag always return true or false, but here we have 3 cases : no arg, last arg is wave64, last arg is no-wave64.

arsenm added inline comments.Sep 30 2020, 8:49 AM
clang/lib/Driver/ToolChains/AMDGPU.cpp
394–395 ↗(On Diff #295297)

hasFlag considers all of these. The features only really need to add +wavefrontsze64 or not based on true or false, the rest is just noise

yaxunl added inline comments.Sep 30 2020, 8:56 AM
clang/lib/Driver/ToolChains/AMDGPU.cpp
394–395 ↗(On Diff #295297)

I have a simpler solution. will update

yaxunl updated this revision to Diff 295312.Sep 30 2020, 8:58 AM

simpler code for handling multiple wave64 options

yaxunl updated this revision to Diff 295326.Sep 30 2020, 9:34 AM

simplifies wavefrontsize64 target feature

arsenm accepted this revision.Oct 1 2020, 6:40 AM
This revision is now accepted and ready to land.Oct 1 2020, 6:40 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 7:18 AM