[OpenMP] Change OMPIRBuilder to append function attributes

[jhuber6]

Summary

Currently the OMPIRBuilder overwrites the function's existing attributes
when it assigns the ones defined in OMPKinds.def. This changes the
behavior to append the current function's attributes with them instead.

[jdoerfert]

can you upload your patches with context please.

[jhuber6]

Adding context, forgot to specify the range.

[jdoerfert]

LG, two nits.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

49

no llvm:: above. call getAttributes only once.

[jhuber6]

Making changes.

[sdmitriev]

With this change OMPIRBuilder may produce invalid IR if existing function attributes conflict with the new attributes it is trying to add. The problem can be reproduced on llvm/test/Transforms/OpenMP/gpu_state_machine_function_ptr_replacement.ll test where __kmpc_global_thread_num function is declared with the following attributes

declare i32 @__kmpc_global_thread_num(%struct.ident_t* nocapture readnone)

With ‘optimistic’ attributes OMPIRBuilder will try to add ‘readonly’ to the function argument which would cause a verification error

$ opt -S -openmpopt -openmp-ir-builder-optimistic-attributes llvm/test/Transforms/OpenMP/gpu_state_machine_function_ptr_replacement.ll
Attributes 'readnone and readonly' are incompatible!
void (%struct.ident_t*, i32)* @__kmpc_barrier_simple_spmd
Attributes 'readnone and readonly' are incompatible!
i32 (%struct.ident_t*)* @__kmpc_global_thread_num
LLVM ERROR: Broken module found, compilation aborted!
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
…

[jhuber6]

In D98740#2684768, @sdmitriev wrote:

With this change OMPIRBuilder may produce invalid IR if existing function attributes conflict with the new attributes it is trying to add. The problem can be reproduced on llvm/test/Transforms/OpenMP/gpu_state_machine_function_ptr_replacement.ll test where __kmpc_global_thread_num function is declared with the following attributes

declare i32 @__kmpc_global_thread_num(%struct.ident_t* nocapture readnone)

With ‘optimistic’ attributes OMPIRBuilder will try to add ‘readonly’ to the function argument which would cause a verification error

$ opt -S -openmpopt -openmp-ir-builder-optimistic-attributes llvm/test/Transforms/OpenMP/gpu_state_machine_function_ptr_replacement.ll
Attributes 'readnone and readonly' are incompatible!
void (%struct.ident_t*, i32)* @__kmpc_barrier_simple_spmd
Attributes 'readnone and readonly' are incompatible!
i32 (%struct.ident_t*)* @__kmpc_global_thread_num
LLVM ERROR: Broken module found, compilation aborted!
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
…

I believe this is a feature. If the attributes aren't compatible then one of them is wrong. I changed the test here, https://reviews.llvm.org/rGf047cb45bd38.

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

 __OMP_RTL_ATTRS(__kmpc_doacross_fini, BarrierAttrs, AttributeSet(),
                 ParamAttrs(ReadOnlyPtrAttrs))
 
-__OMP_RTL_ATTRS(__kmpc_alloc, DefaultAttrs, ReturnPtrAttrs, {})
-__OMP_RTL_ATTRS(__kmpc_free, AllocAttrs, AttributeSet(), {})
-
-__OMP_RTL_ATTRS(__kmpc_init_allocator, DefaultAttrs, ReturnPtrAttrs, {})
-__OMP_RTL_ATTRS(__kmpc_destroy_allocator, AllocAttrs, AttributeSet(), {})
-
-__OMP_RTL_ATTRS(__kmpc_push_target_tripcount_mapper, SetterAttrs, AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_target_mapper, ForkAttrs, AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_target_nowait_mapper, ForkAttrs, AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_target_teams_mapper, ForkAttrs, AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_target_teams_nowait_mapper, ForkAttrs, AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_register_requires, ForkAttrs, AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_target_data_begin_mapper, ForkAttrs, AttributeSet(), {})
+__OMP_RTL_ATTRS(__kmpc_alloc, DefaultAttrs, ReturnPtrAttrs, ParamAttrs())
+__OMP_RTL_ATTRS(__kmpc_free, AllocAttrs, AttributeSet(), ParamAttrs())
+
+__OMP_RTL_ATTRS(__kmpc_init_allocator, DefaultAttrs, ReturnPtrAttrs,
+                ParamAttrs())
+__OMP_RTL_ATTRS(__kmpc_destroy_allocator, AllocAttrs, AttributeSet(),
+                ParamAttrs())
+
+__OMP_RTL_ATTRS(__kmpc_push_target_tripcount_mapper, SetterAttrs,
+                AttributeSet(), ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_target_mapper, ForkAttrs, AttributeSet(), ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_target_nowait_mapper, ForkAttrs, AttributeSet(),
+                ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_target_teams_mapper, ForkAttrs, AttributeSet(),
+                ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_target_teams_nowait_mapper, ForkAttrs, AttributeSet(),
+                ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_register_requires, ForkAttrs, AttributeSet(),
+                ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_target_data_begin_mapper, ForkAttrs, AttributeSet(),
+                ParamAttrs())
 __OMP_RTL_ATTRS(__tgt_target_data_begin_nowait_mapper, ForkAttrs,
-        AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_target_data_end_mapper, ForkAttrs, AttributeSet(), {})
+                AttributeSet(), ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_target_data_end_mapper, ForkAttrs, AttributeSet(),
+                ParamAttrs())
 __OMP_RTL_ATTRS(__tgt_target_data_end_nowait_mapper, ForkAttrs,
-        AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_target_data_update_mapper, ForkAttrs, AttributeSet(), {})
+                AttributeSet(), ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_target_data_update_mapper, ForkAttrs, AttributeSet(),
+                ParamAttrs())
 __OMP_RTL_ATTRS(__tgt_target_data_update_nowait_mapper, ForkAttrs,
-        AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_mapper_num_components, ForkAttrs, AttributeSet(), {})
-__OMP_RTL_ATTRS(__tgt_push_mapper_component, ForkAttrs, AttributeSet(), {})
+                AttributeSet(), ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_mapper_num_components, ForkAttrs, AttributeSet(),
+                ParamAttrs())
+__OMP_RTL_ATTRS(__tgt_push_mapper_component, ForkAttrs, AttributeSet(),
+                ParamAttrs())
 __OMP_RTL_ATTRS(__kmpc_task_allow_completion_event, DefaultAttrs,
                 ReturnPtrAttrs, ParamAttrs(ReadOnlyPtrAttrs))
 

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

 void OpenMPIRBuilder::addAttributes(omp::RuntimeFunction FnID, Function &Fn) {
   LLVMContext &Ctx = Fn.getContext();
 
+  // Get the function's current attributes.
+  auto FnAttrs = Fn.getAttributes().getFnAttributes();
+  auto RetAttrs = Fn.getAttributes().getRetAttributes();
+  llvm::SmallVector<AttributeSet, 4> ArgAttrs;
+  for (size_t ArgNo = 0; ArgNo < Fn.arg_size(); ++ArgNo)
+    ArgAttrs.emplace_back(Fn.getAttributes().getParamAttributes(ArgNo));
+
 #define OMP_ATTRS_SET(VarName, AttrSet) AttributeSet VarName = AttrSet;
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
 
-  // Add attributes to the new declaration.
+  // Add attributes to the function declaration.
   switch (FnID) {
 #define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)                \
-  case Enum:                                                                   \
-    Fn.setAttributes(                                                          \
-        AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));          \
-    break;
+  case Enum: {                                                                 \
+    FnAttrs = FnAttrs.addAttributes(Ctx, FnAttrSet);                           \
+    RetAttrs = RetAttrs.addAttributes(Ctx, RetAttrSet);                        \
+    for (size_t ArgNo = 0; ArgNo < ArgAttrSets.size(); ++ArgNo)                \
+      ArgAttrs[ArgNo] =                                                        \
+          ArgAttrs[ArgNo].addAttributes(Ctx, ArgAttrSets[ArgNo]);              \
+    Fn.setAttributes(AttributeList::get(Ctx, FnAttrs, RetAttrs, ArgAttrs));    \
+    break;                                                                     \
+  }
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
   default:
     // Attributes are optional.

llvm/test/Transforms/OpenMP/add_attributes.ll

 ; RUN: opt < %s -S -passes=openmpopt | FileCheck %s
 ; RUN: opt < %s -S -openmpopt        -openmp-ir-builder-optimistic-attributes | FileCheck %s --check-prefix=OPTIMISTIC
 ; RUN: opt < %s -S -passes=openmpopt -openmp-ir-builder-optimistic-attributes | FileCheck %s --check-prefix=OPTIMISTIC
-;
-; TODO: Not all omp_XXXX methods are known to the OpenMPIRBuilder/OpenMPOpt.
-;
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 %struct.omp_lock_t = type { i8* }

 
 declare dso_local i32 @omp_get_max_active_levels()
 
-declare dso_local void @omp_get_schedule(i32*, i32*)
+declare dso_local void @omp_get_schedule(i32* nocapture, i32* nocapture)
 
 declare dso_local i32 @omp_get_max_task_priority()
 

 
 declare void @__kmpc_proxy_task_completed_ooo(i8*)
 
+; Function Attrs: noinline
+declare void @__kmpc_barrier_simple_spmd(%struct.ident_t* nocapture nofree readonly, i32) #0
+
+attributes #0 = { noinline }
+
 ; CHECK: ; Function Attrs: nounwind
 ; CHECK-NEXT: declare dso_local void @omp_set_num_threads(i32)
 

 ; CHECK-NEXT: declare dso_local void @omp_set_schedule(i32, i32)
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_num_threads() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_num_threads()
 
 ; CHECK-NOT: Function Attrs
 ; CHECK: declare dso_local void @use_int(i32)
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_dynamic() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_dynamic()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_nested() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_nested()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_max_threads() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_max_threads()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_thread_num() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_thread_num()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_num_procs() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_num_procs()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_in_parallel() #0
+; CHECK-NEXT: declare dso_local i32 @omp_in_parallel()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_in_final() #0
+; CHECK-NEXT: declare dso_local i32 @omp_in_final()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_active_level() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_active_level()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_level() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_level()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_ancestor_thread_num(i32) #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_ancestor_thread_num(i32)
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_team_size(i32) #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_team_size(i32)
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_thread_limit() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_thread_limit()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_max_active_levels() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_max_active_levels()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local void @omp_get_schedule(i32* nocapture writeonly, i32* nocapture writeonly) #0
+; CHECK-NEXT: declare dso_local void @omp_get_schedule(i32* nocapture writeonly, i32* nocapture writeonly)
 
 ; CHECK-NOT: Function Attrs
 ; CHECK: declare dso_local i32 @omp_get_max_task_priority()

 ; CHECK: declare dso_local i32 @omp_get_team_num()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_cancellation() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_cancellation()
 
 ; CHECK-NOT: Function Attrs
 ; CHECK: declare dso_local i32 @omp_get_initial_device()

 ; CHECK: declare dso_local i32 @omp_get_device_num()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_proc_bind() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_proc_bind()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_num_places() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_num_places()
 
 ; CHECK-NOT: Function Attrs
 ; CHECK: declare dso_local i32 @omp_get_place_num_procs(i32)
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local void @omp_get_place_proc_ids(i32, i32* nocapture writeonly) #0
+; CHECK-NEXT: declare dso_local void @omp_get_place_proc_ids(i32, i32* nocapture writeonly)
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_place_num() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_place_num()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local i32 @omp_get_partition_num_places() #0
+; CHECK-NEXT: declare dso_local i32 @omp_get_partition_num_places()
 
 ; CHECK: ; Function Attrs: nounwind
-; CHECK-NEXT: declare dso_local void @omp_get_partition_place_nums(i32*) #0
+; CHECK-NEXT: declare dso_local void @omp_get_partition_place_nums(i32*)
 
 ; CHECK-NOT: Function Attrs
 ; CHECK: declare dso_local i32 @omp_control_tool(i32, i32, i8*)

 ; CHECK: ; Function Attrs: nounwind
 ; CHECK-NEXT: declare void @__kmpc_proxy_task_completed_ooo(i8*)
 
+; CHECK: ; Function Attrs: convergent noinline nounwind
+; CHECK-NEXT: declare void @__kmpc_barrier_simple_spmd(%struct.ident_t* nocapture nofree readonly, i32)
+
 ; OPTIMISTIC: ; Function Attrs: inaccessiblememonly nofree nosync nounwind willreturn writeonly
 ; OPTIMISTIC-NEXT: declare dso_local void @omp_set_num_threads(i32)
 

 
 ; OPTIMISTIC: ; Function Attrs: nofree nosync nounwind willreturn
 ; OPTIMISTIC-NEXT: declare void @__kmpc_proxy_task_completed_ooo(i8*)
+
+; OPTIMISTIC: ; Function Attrs: convergent noinline nounwind 
+; OPTIMISTIC-NEXT: declare void @__kmpc_barrier_simple_spmd(%struct.ident_t* nocapture nofree readonly, i32)
+