[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 6 inline comments as done.
yaxunl added inline comments.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:9481
+  // emitted, Cand1 is not better than Cand2. This rule should have precedence
+  // over other rules.
+  //
----------------
rjmccall wrote:
> Please add `[CUDA]` or something similar to the top of this comment so that readers can immediately know that it's dialect-specific.
> 
> At a high level, this part of the rule is essentially saying that CUDA non-emittability is a kind of non-viability.  Should we just make non-emittable functions get flagged as non-viable (which will avoid a lot of relatively expensive conversion checking), or is it important to be able to select non-emittable candidates over candidates that are non-viable for other reasons?
There are two situations for "bad" callees:

1. the callee should never be called. It is not just invalid call in codegen, but also invalid call in AST. e.g. a host function call a device function. In CUDA call preference, it is termed "never". And clang already removed such callees from overload candidates.

2. the callee should not be called in codegen, but may be called in AST. This happens with `__host__ __device__` functions when calling a "wrong sided" function. e.g. in device compilation, a `__host__ __device__` function calls a `__host__` function. This is valid in AST since the `__host__ __device__` function may be an inline function which is only called by a `__host__` function. There is a deferred diagnostic for the wrong-sided call, which is triggered only if the caller is emitted. However in overloading resolution, if no better candidates are available, wrong-sided candidates are still viable.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:9749
+  if (isBetterMultiversionCandidate(Cand1, Cand2))
+    return true;
+
----------------
rjmccall wrote:
> If we move anything below this check, it needs to figure out a tri-state so that it can return false if `Cand2` is a better candidate than `Cand1`.  Now, that only matters if multiversion functions are supported under CUDA, but if you're relying on them not being supported, that should at least be commented on.
multiversion host functions is orthogonal to CUDA therefore should be supported. multiversion in device, host device, and global functions are not supported. However this change does not make things worse, and should continue to work if they are supported.

host/device based overloading resolution is mostly for determining viability of a function. If two functions are both viable, other factors should take precedence in preference. This general rule has been taken for cases other than multiversion, I think it should also apply to multiversion.

I will make isBetterMultiversionCandidate three states.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:9752
+  // If other rules cannot determine which is better, CUDA preference is used
+  // to determine which is better.
+  if (S.getLangOpts().CUDA && Cand1.Function && Cand2.Function) {
----------------
rjmccall wrote:
> Okay, let's think about the right place to put this check in the ordering; we don't want different extensions to get into a who-comes-last competition.
> 
> - Certainly this should have lower priority than the standard-defined preferences like argument conversion ranks or `enable_if` partial-ordering.
> - The preference for pass-object-size parameters is probably most similar to a type-based-overloading decision and so should take priority.
> - I would say that this should take priority over function multi-versioning.  Function multi-versioning is all about making specialized versions of the "same function", whereas I think host/device overloading is meant to be semantically broader than that.
> 
> What do you think?
> 
> Regardless, the rationale for the order should be explained in comments.
I will add comments for the rationale of preference.

I commented the preference between multiversion and host/device in another comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77954/new/

https://reviews.llvm.org/D77954