Page MenuHomePhabricator

[HIP] Warn no --offload-arch option
AbandonedPublic

Authored by yaxunl on Nov 30 2020, 9:14 PM.

Details

Reviewers
tra
Summary

This patch let clang emit a warning when no --offload-arch option is specified, which
usually indicates that users forget to specify this option.

Diff Detail

Event Timeline

yaxunl requested review of this revision.Nov 30 2020, 9:14 PM
yaxunl created this revision.
tra added a comment.Dec 1 2020, 12:23 PM

While I agree that the default GPU choice is not likely to be correct, or usable, for everyone, but the warning seems to be a half-measure.
If the default is not usable, then it should not be the default. If it's usable, then we don't need a warning.

Having a warning would make sense if it were a part of the plan to transition towards making GPU arch a mandatory option. Is that the case here?
Just a warning is not very useful, IMO. The users would need to specify the GPU in order to silence it, so why not just require it.

In D92363#2426401, @tra wrote:

While I agree that the default GPU choice is not likely to be correct, or usable, for everyone, but the warning seems to be a half-measure.
If the default is not usable, then it should not be the default. If it's usable, then we don't need a warning.

Having a warning would make sense if it were a part of the plan to transition towards making GPU arch a mandatory option. Is that the case here?
Just a warning is not very useful, IMO. The users would need to specify the GPU in order to silence it, so why not just require it.

There are still valid use cases for not providing GPU arch. For example, users would like to test syntax of a HIP program for which GPU arch does not matter, or users would like to get PCH file where GPU arch does not matter. Another example is cmake may test whether a compiler can compile HIP program without options. Make it a warning allows such uses cases.

tra added a comment.Dec 3 2020, 12:17 PM

There are still valid use cases for not providing GPU arch. For example, users would like to test syntax of a HIP program for which GPU arch does not matter, or users would like to get PCH file where GPU arch does not matter. Another example is cmake may test whether a compiler can compile HIP program without options. Make it a warning allows such uses cases.

Issuing no warning allows such use cases even better. :-)

Also, how do I silence that warning, if I'm compiling with -Werror?
If we provide a knob to silence the warning, how would that be better than asking the user to specify a GPU arch?
If we do ask users to specify GPU arch in order to silence the warning => we effectively make the GPU arch a mandatory option.

I believe that no warning or a mandatory GPU arch would be a more principled way. With the warning we effectively flag the default value as 'it's not a very useful default'. If it's not useful by default, then it should not be the default. If it is useful, there should be no warning.

In this case, I think making GPU arch mandatory would have a stronger case, as the default we use right now is indeed quite often wrong. However, we probably have existing users who have valid use cases that work without the explicitly specified GPU arch, so requiring the GPU arch would break them. That constrains us to the no-warning scenario in practice.

Introducing a warning would break existing users, too -- compiling with -Werror is not that uncommon. If we're willing to break users, we may as well break them by requiring a GPU arch.
I just don't see a compelling argument where adding a warning would provide a net benefit over no-warning or a mandatory-GPU-arch scenarios.

yaxunl abandoned this revision.Jan 15 2021, 1:02 PM