This is an archive of the discontinued LLVM Phabricator instance.

[HIP] clang should pass `-mno-amdgpu-ieee` to -cc1
ClosedPublic

Authored by yaxunl on Mar 9 2023, 12:44 PM.

Diff Detail

Event Timeline

yaxunl created this revision.Mar 9 2023, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 12:44 PM
yaxunl requested review of this revision.Mar 9 2023, 12:44 PM
tra added a comment.Mar 9 2023, 1:44 PM

clang should pass -mno-amdgpu-ieee to -cc1

It would be useful to have some details on why we should pass that option.

clang/test/Driver/hip-options.hip
133

Nit: Both lines could be collapsed into matching "-m{{(no-)?}}amdgpu-ieee" Either way is fine.

137–138

This looks odd.
If the DAG line matches on the first clang invocation, the NEG-NOT will always succeed, because it will have nothing to check.

If clang invocation with -no-mamdgpu-ieee happens to be before the invocation with -mamdgpu-ieee, then the NEG-NOT will not succeed.

Do I understand it correctly that the idea here is to make sure that only -mno-amdgpu-ieee is ever passed to cc1?
What's the purpose of -DAG? Do you expect to see multiple cc1 with "-triple" "amdgcn-amd-amdhsa" ? AFAICT there should be only one for gfx906 and DAG should not be needed.

The -NOT check I'd do in a separate RUN.

clang should pass -mno-amdgpu-ieee to -cc1

It would be useful to have some details on why we should pass that option.

-mamdgpu-ieee was introduced by https://reviews.llvm.org/D77013 but I forgot to let clang driver pass it to clang -cc1. Recently we found that.

clang/test/Driver/hip-options.hip
133

will do

137–138

The -NEG check is supposed to be a separate run. I will add a separate run for it and remove -DAG

yaxunl updated this revision to Diff 506296.Mar 18 2023, 8:38 AM

fix tests

tra accepted this revision.Mar 20 2023, 10:07 AM
This revision is now accepted and ready to land.Mar 20 2023, 10:07 AM
This revision was landed with ongoing or failed builds.Apr 2 2023, 5:46 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2023, 5:46 AM