[PATCH] D67509: [CUDA][HIP] Fix hostness of defaulted constructor

Thu Sep 19 10:26:15 PDT 2019

tra accepted this revision.
tra added inline comments.
This revision is now accepted and ready to land.

================
Comment at: lib/Sema/SemaCUDA.cpp:273-274
+                 MemberDecl->hasAttr<CUDAHostAttr>();
+  if (!InClass || hasAttr)
+    return false;
+
----------------
yaxunl wrote:
> tra wrote:
> > A comment here would be helpful.
> > 
> > I think the intent here is to look for implicit special members with *explicitly* set attributes.
> > We have number of cases where we set H/D attributes implicitly. I'm not sure whether we ever see any of them here, but if we do, it will sneak through this check. I think a check for whether the attribute is explicit would be prudent.
> will add the comment.
> 
> I intentionally omitted check for explicit attr because I noticed the same special member is inferred twice. Each time it is added the same attrs, which cause them to have two `__host__` and two `__device__` attrs. By checking if attrs exist (not just explicit attrs) we can avoid duplicate attrs. I tested this with real machine learning frameworks and did not see issues.
OK. So, if we have explicit attributes, then there's no need to infer them. If the attributes are implicit, then we've already guessed them, so there's no point doing it again. The check for simple attribute presence covers both cases. I'll buy that. 

This leaves a hypothetical gap in case we have implicitly set attributes set somewhere else that would disagree with the attributes that would be set by this function.  It would be great to have an assertion somewhere to verify that it does not happen. Alas, this function modifies the MemberDecl, so I don't see an easy way to do it.

In general, the multiple application of the attributes seems to be a separate issue that should be fixed. I think we run into it in other places, too. 

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

https://reviews.llvm.org/D67509