[PATCH] D78655: [HIP] Add -fhip-lambda-host-device

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

There are two behaviors that seem to make sense:

- Treat lambdas as implicitly HD (like constexpr functions) in all CUDA / HIP language modes. I don't think it makes sense for lambdas to become implicitly HD in C++17 simply because they become implicitly `constexpr`, nor for their HDness to depend on whether their parameter types happen to be literal types, etc. So in C++17, where lambdas are constexpr whenever they can be, the logical behavior would seem to be that lambdas are implicitly HD. And then for consistency with that, I'd expect them to be implicitly HD across all language modes.
- Implicitly give lambdas the same HD-ness as the enclosing function (if there is one).

I would think the best choice may be to do both of these things: if there is an enclosing function, inherit its host or device attributes. And if not, then treat the lambda as implicitly HD. A slight variation on that, that might be better: lambdas with no lambda-capture are implicitly HD; lambdas with any lambda-capture (which must therefore have an enclosing function) inherit the enclosing function's HDness.

(Note that if we go this way, it makes no difference if there are reference captures, because they're always references on the same "side".)



================
Comment at: clang/include/clang/Basic/LangOptions.def:247
 LANGOPT(HIPUseNewLaunchAPI, 1, 0, "Use new kernel launching API for HIP")
+LANGOPT(HIPLambdaHostDevice, 1, 0, "Let non-reference-capturing lambda be host device for HIP")
 
----------------
Is it really appropriate to have a flag for this? I would have expected that either the correct HIP behavior would be that lambdas are implicitly HD, or not, and Clang should just use whichever behavior is correct. (I don't know what authority decides what is and is not "correct" for HIP, but still.)

If this is a difference between versions of HIP, we generally model that by having a single "version" field rather than ways of turning on/off the individual changes.


================
Comment at: clang/include/clang/Driver/Options.td:628-632
+def fhip_lambda_host_device : Flag<["-"], "fhip-lambda-host-device">,
+  Flags<[CC1Option]>,
+  HelpText<"Let a lambda function without host/device attributes be a host "
+  "device function if it does not capture by reference (HIP only)">;
+def fno_hip_lambda_host_device : Flag<["-"], "fno-hip-lambda-host-device">;
----------------
You document these as "Let [lambdas be HD]" (which I would understand to mean "permit lambdas to be HD"), but the actual effect appears to be "make lambdas be HD by default".


================
Comment at: clang/lib/Sema/SemaCUDA.cpp:728-730
   FunctionDecl *CurFn = dyn_cast<FunctionDecl>(CurContext);
   if (!CurFn)
     return;
----------------
This check appears to prevent lambdas appearing in any context outside a function from being implicitly HD. Is that what you want? Eg:

```
auto foo = [] {}; // not implicitly HD
```


================
Comment at: clang/lib/Sema/SemaCUDA.cpp:733
+      (Method->isConstexpr() ||
+       (getLangOpts().HIPLambdaHostDevice && !LambdaHasRefCaptures(LI)))) {
+    Method->addAttr(CUDADeviceAttr::CreateImplicit(Context));
----------------
The reference captures check seems quite strange to me. A copy capture of a pointer could have the same problem, as could a copy capture of a class that contains a reference or a pointer. As could an init-capture.

These kinds of quirky language rules are usually more trouble than they're worth.


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

https://reviews.llvm.org/D78655