This is an archive of the discontinued LLVM Phabricator instance.

[CUDA][HIP] Fix device template variables
ClosedPublic

Authored by yaxunl on May 11 2021, 12:07 PM.

Details

Reviewers
tra
Summary

Currently clang does not emit device template variables
instantiated only in host functions, however, nvcc is
able to do that:

https://godbolt.org/z/fneEfferY

This patch fixes this issue by refactoring and extending
the existing mechanism for emitting static device
var ODR-used by host only. Basically clang records
device variables ODR-used by host code and force
them to be emitted in device compilation. The existing
mechanism makes sure these device variables ODR-used
by host code are added to llvm.compiler-used, therefore
they are guaranteed not to be deleted.

This patch also fixes non-ODR-use of static device
variables by host code causing static device variables
emitted.

Diff Detail

Event Timeline

yaxunl requested review of this revision.May 11 2021, 12:07 PM
yaxunl created this revision.
tra accepted this revision.May 11 2021, 12:41 PM

LGTM in general.
Perhaps it would make sense to combine this patch with D102237 as both patches are changing the same code for the same reason, just for slightly different kinds of variables.

This revision is now accepted and ready to land.May 11 2021, 12:41 PM

LGTM in general.
Perhaps it would make sense to combine this patch with D102237 as both patches are changing the same code for the same reason, just for slightly different kinds of variables.

Will merge D102237 into this one.

yaxunl updated this revision to Diff 344605.May 11 2021, 5:20 PM

Revised by Artem's comments

yaxunl updated this revision to Diff 344629.May 11 2021, 6:34 PM
yaxunl edited the summary of this revision. (Show Details)

fix lint warning

tra accepted this revision.May 12 2021, 10:07 AM
tra added inline comments.
clang/lib/Sema/SemaExpr.cpp
17145

Revisiting my comment from https://reviews.llvm.org/D102237#inline-967732

I think this can be combined with the if (SemaRef.LangOpts.CUDA) above.

Unless I'm missing something obvious, what we have now is

if (SemaRef.LangOpts.CUDA) {
  <vars>
  if (Var && Var->hasGlobalStorage()) {
    
  }
}

You've mentioned that they can't be combined because of an else, but there's no else in the if (Var && Var->hasGlobalStorage()) as far as I can see, and <vars> could be moved inside it.

yaxunl closed this revision.May 12 2021, 2:14 PM
yaxunl marked an inline comment as done.
yaxunl added inline comments.
clang/lib/Sema/SemaExpr.cpp
17145

You are right. I missed if (Var && Var->hasGlobalStorage()). The patch has been committed. Will make the change by a commit directly. Thanks.