This is an archive of the discontinued LLVM Phabricator instance.

[CUDA][HIP] Fix capturing reference to host variable
ClosedPublic

Authored by yaxunl on Nov 9 2020, 9:32 AM.

Details

Summary

In C++ when a reference variable is captured by copy, the lambda
is supposed to make a copy of the referenced variable in the captures
and refer to the copy in the lambda. Therefore, it is valid to capture
a reference to a host global variable in a device lambda since the
device lambda will refer to the copy of the host global variable instead
of access the host global variable directly.

However, clang tries to avoid capturing of reference to a host global variable
if it determines the use of the reference variable in the lambda function is
not odr-use. Clang also tries to emit load of the reference to a global variable
as load of the global variable if it determines that the reference variable is
a compile-time constant.

For a device lambda to capture a reference variable to host global variable
and use the captured value, clang needs to be taught that in such cases the use of the reference
variable is odr-use and the reference variable is not compile-time constant.

This patch fixes that.

Diff Detail

Event Timeline

yaxunl requested review of this revision.Nov 9 2020, 9:32 AM
yaxunl created this revision.
yaxunl updated this revision to Diff 303905.Nov 9 2020, 9:39 AM

remove debug code

tra added a subscriber: rsmith.Nov 9 2020, 10:19 AM

The use case in the tests makes sense, but I'd like to have a second opinion on whether it may have unintended consequences. Lambdas tend to bring interesting corner cases.

@rsmith Richard, do you see any issues with this approach?

clang/test/CodeGenCUDA/lambda-reference-var.cu
62

Do we have current Sema tests that verify that we we would not allow accessing host vars here?

yaxunl added inline comments.Nov 9 2020, 11:06 AM
clang/test/CodeGenCUDA/lambda-reference-var.cu
62

currently clang does not diagnose access to host global variable in device functions. do we want to diagnose that?

tra added a comment.Nov 9 2020, 11:33 AM

Ouch. This is definitely a bug. https://godbolt.org/z/faPWcv

Granted, the compilation will eventually fail in most cases due to ptxas failing to resolve the reference to the variable we do not generate on the device, but it's something we should catch in Sema.

Filed https://bugs.llvm.org/show_bug.cgi?id=48121

yaxunl updated this revision to Diff 304494.Nov 11 2020, 5:45 AM
yaxunl edited the summary of this revision. (Show Details)

added diagnosing referencing host variable in device functions

tra added a comment.Nov 11 2020, 9:50 AM

added diagnosing referencing host variable in device functions

Thank you for fixing this. The new changes look good, but I think they should be in a separate patch.

clang/lib/Sema/SemaExpr.cpp
357

This could use a comment about why we only check D->H references.
H->D references will still work (sort of) because on the host side we have matching shadow variables.

yaxunl updated this revision to Diff 304566.Nov 11 2020, 10:05 AM
yaxunl edited the summary of this revision. (Show Details)

Separate diagnose of host variable to another patch.

yaxunl marked 2 inline comments as done.Nov 24 2020, 5:39 AM

ping

clang/lib/Sema/SemaExpr.cpp
357

the comment has been added in the separated patch for diagnostics.

tra added a comment.Nov 30 2020, 1:23 PM

LGTM in general.

clang/lib/Sema/SemaExpr.cpp
1959

Nit: I'd make it a free function.
The large-ish lambda handling a niche case makes the original simple function less readable. I think keeping capture check separate would be a bit cleaner.

1997

This could use some Sema tests.

yaxunl updated this revision to Diff 308519.Nov 30 2020, 8:10 PM
yaxunl marked an inline comment as done.

extract lambda as a function

yaxunl marked 2 inline comments as done.Nov 30 2020, 8:17 PM
yaxunl added inline comments.
clang/lib/Sema/SemaExpr.cpp
1959

done

1997

the negative cases of the condition that incur diagnostics are referencing host variable in device or host device functions. The sema tests have been added in https://reviews.llvm.org/D91281.

The other negative cases of the condition (capturing host reference in host lambda, referencing host var in host function, capturing device reference in host lambda, referencing device var in host function, etc) do not cause diagnostics, which are covered by codegen tests.

tra accepted this revision.Dec 1 2020, 12:40 PM
tra added inline comments.
clang/lib/Sema/SemaExpr.cpp
1954

const Sema& ?

This revision is now accepted and ready to land.Dec 1 2020, 12:40 PM
yaxunl marked 3 inline comments as done.Dec 2 2020, 6:43 AM
yaxunl added inline comments.
clang/lib/Sema/SemaExpr.cpp
1954

will do when committing

This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 7:15 AM