[OpenMP] Always apply target declarations to canonical definitions

[jhuber6]

Summary

This patch changes the handling of OpenMP to add the device attributes
to the canonical definitions when we encounter a non-canonical
definition. Previously, the following code would not work because it
would find the non-canonical definition first which would then not be
used anywhere else.

int x;
extern int x;

This patch now adds the attribute to both of them. This allows us to
perform the following operation if, for example, there were an
implementation of stderr on the device.

#include <stdio.h>

// List of libc symbols supported on the device.
extern FILE *stderr;

Unfortunately I cannot think of an equivalent solution to HIP / CUDA
device declarations as those are done with simple attributes. Attributes
themselves cannot be used to affect a definition once its canonical
definition has already been seen. Some help on that front would be
appreciated.

Fixes https://github.com/llvm/llvm-project/issues/63355

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

23099 ↗

(On Diff #532979)

Need to use ML->DeclarationMarkedOpenMPDeclareTarget for CD too.

[jhuber6]

Adding AST mutation listener to the other modified declaration to signal that it was changed.

[jhuber6]

Fix logic

[jhuber6]

ping

[ABataev]

Did you try instead fix the OMPDeclareTargetDeclAttr::getActiveAttr() function to make it look through all the declarations and return the attribute from the first found instead of adding a new attribute?

clang/lib/Sema/SemaOpenMP.cpp

22993–22999 ↗

(On Diff #533002)

Here it would be better to reconstruct the attribute and make it implicit attribute.

23105–23112 ↗

(On Diff #533002)

Here it would be better to reconstruct the attribute and make it implicit attribute

[jhuber6]

In D153369#4451993, @ABataev wrote:

Did you try instead fix the OMPDeclareTargetDeclAttr::getActiveAttr() function to make it look through all the declarations and return the attribute from the first found instead of adding a new attribute?

I originally tried that but found that once we've found a canonical declaration, nothing will really bind to the new non-canonical definition. So the only way to do it would be to scan the entire file with the source manager as far as I could tell. I could be wrong though, I'm not as familiar with Clang here.

clang/lib/Sema/SemaOpenMP.cpp

23105–23112 ↗

(On Diff #533002)

I thought it was already implcit since we use OMPDeclareTargetDeclAttr::CreateImplicit above, what would be different in the new version?

[ABataev]

In D153369#4452008, @jhuber6 wrote:

In D153369#4451993, @ABataev wrote:

Did you try instead fix the OMPDeclareTargetDeclAttr::getActiveAttr() function to make it look through all the declarations and return the attribute from the first found instead of adding a new attribute?

I originally tried that but found that once we've found a canonical declaration, nothing will really bind to the new non-canonical definition. So the only way to do it would be to scan the entire file with the source manager as far as I could tell. I could be wrong though, I'm not as familiar with Clang here.

Hmm, it should not be so. Did you try to get redecls()?

[jhuber6]

Updating to use VD->redecls().

Thanks for pointing that out, couldn't find it when I looked initially.

[ABataev]

LG with a nit

clang/lib/Sema/SemaOpenMP.cpp

22991 ↗

(On Diff #534950)

Remove new line here

clang/lib/AST/AttrImpl.cpp

if (Expr *E = getIndirectExpr()) {
     OS << ")";
   } else if (getIndirect()) {
     OS << " indirect";
   }
 }
 
 std::optional<OMPDeclareTargetDeclAttr *>
 OMPDeclareTargetDeclAttr::getActiveAttr(const ValueDecl *VD) {
-  if (!VD->hasAttrs())
+  if (llvm::all_of(VD->redecls(), [](const Decl *D) { return !D->hasAttrs(); }))
     return std::nullopt;
   unsigned Level = 0;
   OMPDeclareTargetDeclAttr *FoundAttr = nullptr;
-  for (auto *Attr : VD->specific_attrs<OMPDeclareTargetDeclAttr>()) {
+  for (const Decl *D : VD->redecls()) {
+    for (auto *Attr : D->specific_attrs<OMPDeclareTargetDeclAttr>()) {
       if (Level <= Attr->getLevel()) {
         Level = Attr->getLevel();
         FoundAttr = Attr;
       }
     }
+  }
   if (FoundAttr)
     return FoundAttr;
   return std::nullopt;
 }
 
 std::optional<OMPDeclareTargetDeclAttr::MapTypeTy>
 OMPDeclareTargetDeclAttr::isDeclareTargetDeclaration(const ValueDecl *VD) {
   std::optional<OMPDeclareTargetDeclAttr *> ActiveAttr = getActiveAttr(VD);

clang/test/OpenMP/declare_target_codegen.cpp

 
 // SIMD-ONLY-NOT: {{__kmpc|__tgt}}
 // KMPC-ONLY-NOT: __tgt
 
 // CHECK-NOT: define {{.*}}{{baz1|baz4|maini1|Base|virtual_}}
 // CHECK-DAG: Bake
 // CHECK-NOT: @{{hhh|ggg|fff|eee}} =
 // CHECK-DAG: @flag = protected global i8 undef,
+// CHECK-DAG: @dx = {{protected | }}global i32 0,
+// CHECK-DAG: @dy = {{protected | }}global i32 0,
 // CHECK-DAG: @aaa = external global i32,
-// CHECK-DAG: @bbb ={{ protected | }}global i32 0,
+// CHECK-DAG: @bbb = {{protected | }}global i32 0,
 // CHECK-DAG: weak constant %struct.__tgt_offload_entry { ptr @bbb,
 // CHECK-DAG: @ccc = external global i32,
-// CHECK-DAG: @ddd ={{ protected | }}global i32 0,
+// CHECK-DAG: @ddd = {{protected | }}global i32 0,
 // CHECK-DAG: @hhh_decl_tgt_ref_ptr = weak global ptr null
 // CHECK-DAG: @ggg_decl_tgt_ref_ptr = weak global ptr null
 // CHECK-DAG: @fff_decl_tgt_ref_ptr = weak global ptr null
 // CHECK-DAG: @eee_decl_tgt_ref_ptr = weak global ptr null
 // CHECK-DAG: @{{.*}}maini1{{.*}}aaa = internal global i64 23,
 // CHECK-DAG: @pair = {{.*}}addrspace(3) global %struct.PAIR undef
 // CHECK-DAG: @_ZN2SS3SSSE ={{ protected | }}global i32 1,
 // CHECK-DAG: @b ={{ protected | }}global i32 15,
 // CHECK-DAG: @d ={{ protected | }}global i32 0,
 // CHECK-DAG: @c = external global i32,
 // CHECK-DAG: @globals ={{ protected | }}global %struct.S zeroinitializer,
 // CHECK-DAG: [[STAT:@.+stat]] = internal global %struct.S zeroinitializer,
 // CHECK-DAG: [[STAT_REF:@.+]] = internal constant ptr [[STAT]]
 // CHECK-DAG: @out_decl_target ={{ protected | }}global i32 0,
 // CHECK-DAG: @llvm.compiler.used = appending global [1 x ptr] [ptr [[STAT_REF]]],
 
 // CHECK-DAG: define {{.*}}i32 @{{.*}}{{foo|bar|baz2|baz3|FA|f_method}}{{.*}}()
 // CHECK-DAG: define {{.*}}void @{{.*}}TemplateClass{{.*}}(ptr {{[^,]*}} %{{.*}})
 // CHECK-DAG: define {{.*}}i32 @{{.*}}TemplateClass{{.*}}f_method{{.*}}(ptr {{[^,]*}} %{{.*}})
-// CHECK-DAG: define {{.*}}void @__omp_offloading_{{.*}}_globals_l[[@LINE+78]]_ctor()
+// CHECK-DAG: define {{.*}}void @__omp_offloading_{{.*}}_globals_l[[@LINE+89]]_ctor()
 
 #ifndef HEADER
 #define HEADER
+
+int dx = 0;
+extern int dx;
+#pragma omp declare target to(dx)
+
+int dy = 0;
+#pragma omp begin declare target
+
+extern int dy;
+#pragma omp end declare target
+
 #pragma omp declare target
 bool flag [[clang::loader_uninitialized]];
 extern int bbb;
 #pragma omp end declare target
 #pragma omp declare target
 extern int bbb;
 #pragma omp end declare target