[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_init, BarrierAttrs, AttributeSet(),
                 ParamAttrs(ReadOnlyPtrAttrs))
 __OMP_RTL_ATTRS(__kmpc_doacross_post, BarrierAttrs, AttributeSet(),
                 ParamAttrs(ReadOnlyPtrAttrs, AttributeSet(), ReadOnlyPtrAttrs))
 __OMP_RTL_ATTRS(__kmpc_doacross_wait, BarrierAttrs, AttributeSet(),
                 ParamAttrs(ReadOnlyPtrAttrs, AttributeSet(), ReadOnlyPtrAttrs))
 __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_alloc, DefaultAttrs, ReturnPtrAttrs, ParamAttrs())
+__OMP_RTL_ATTRS(__kmpc_free, AllocAttrs, AttributeSet(), ParamAttrs())
 
-__OMP_RTL_ATTRS(__kmpc_init_allocator, DefaultAttrs, ReturnPtrAttrs, {})
-__OMP_RTL_ATTRS(__kmpc_destroy_allocator, AllocAttrs, AttributeSet(), {})
+__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(), {})
-__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_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))
 
 #undef __OMP_RTL_ATTRS
 #undef OMP_RTL_ATTRS
 #undef AttributeSet
 #undef EnumAttr
 #undef EnumAttrInt

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

static cl::opt<bool>
     OptimisticAttributes("openmp-ir-builder-optimistic-attributes", cl::Hidden,
                          cl::desc("Use optimistic attributes describing "
                                   "'as-if' properties of runtime calls."),
                          cl::init(false));
 
 void OpenMPIRBuilder::addAttributes(omp::RuntimeFunction FnID, Function &Fn) {
   LLVMContext &Ctx = Fn.getContext();
 
+  // Get the function's current attributes.
+  auto Attrs = Fn.getAttributes();
+  auto FnAttrs = Attrs.getFnAttributes();
+  auto RetAttrs = Attrs.getRetAttributes();
+  SmallVector<AttributeSet, 4> ArgAttrs;
+  for (size_t ArgNo = 0; ArgNo < Fn.arg_size(); ++ArgNo)
+    ArgAttrs.emplace_back(Attrs.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));          \
+    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.
     break;
   }
 }
 

llvm/test/Transforms/OpenMP/add_attributes.ll

 ; RUN: opt < %s -S -openmpopt        | FileCheck %s
 ; 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* }
 %struct.omp_nest_lock_t = type { i8* }
 %struct.ident_t = type { i32, i32, i32, i32, i8* }
 
 define void @call_all(i32 %schedule, %struct.omp_lock_t* %lock, i32 %lock_hint, %struct.omp_nest_lock_t* %nest_lock, i32 %i, i8* %s, i64 %st, i8* %vp, double %d, i32 %proc_bind, i64 %allocator_handle, i8* %cp, i64 %event_handle, i32 %pause_resource) {
 entry:

 declare i8* @__kmpc_task_reduction_get_th_data(i32, i8*, i8*)
 
 declare i8* @__kmpc_task_reduction_init(i32, i32, i8*)
 
 declare i8* @__kmpc_task_reduction_modifier_init(i8*, i32, i32, i32, i8*)
 
 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: ; Function Attrs: nounwind
 ; CHECK-NEXT: declare dso_local void @omp_set_dynamic(i32)
 
 ; CHECK: ; Function Attrs: nounwind
 ; CHECK-NEXT: declare dso_local void @omp_set_nested(i32)
 
 ; CHECK: ; Function Attrs: nounwind
 ; CHECK-NEXT: declare dso_local void @omp_set_max_active_levels(i32)
 
 ; CHECK: ; Function Attrs: nounwind
 ; 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-NOT: Function Attrs
 ; CHECK: declare dso_local void @omp_init_lock(%struct.omp_lock_t*)
 
 ; CHECK-NOT: Function Attrs

 
 ; CHECK-NOT: Function Attrs
 ; CHECK: declare dso_local i32 @omp_get_num_teams()
 
 ; CHECK-NOT: Function Attrs
 ; 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-NOT: Function Attrs
 ; CHECK: declare dso_local i8* @omp_target_alloc(i64, i32)
 
 ; CHECK-NOT: Function Attrs

 
 ; CHECK-NOT: Function Attrs
 ; CHECK: declare dso_local i32 @omp_target_disassociate_ptr(i8*, i32)
 
 ; CHECK-NOT: Function Attrs
 ; 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-NOT: Function Attrs
 ; CHECK: declare dso_local void @omp_destroy_allocator(i64)
 
 ; CHECK-NOT: Function Attrs

 ; CHECK-NEXT: declare i8* @__kmpc_task_reduction_init(i32, i32, i8*)
 
 ; CHECK: ; Function Attrs: nounwind
 ; CHECK-NEXT: declare i8* @__kmpc_task_reduction_modifier_init(i8*, i32, 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: inaccessiblememonly nofree nosync nounwind willreturn writeonly
 ; OPTIMISTIC-NEXT: declare dso_local void @omp_set_dynamic(i32)
 
 ; OPTIMISTIC: ; Function Attrs: inaccessiblememonly nofree nosync nounwind willreturn writeonly
 ; OPTIMISTIC-NEXT: declare dso_local void @omp_set_nested(i32)

 ; OPTIMISTIC: ; Function Attrs: nofree nosync nounwind willreturn
 ; OPTIMISTIC-NEXT: declare noalias i8* @__kmpc_task_reduction_init(i32, i32, i8*)
 
 ; OPTIMISTIC: ; Function Attrs: nofree nosync nounwind willreturn
 ; OPTIMISTIC-NEXT: declare noalias i8* @__kmpc_task_reduction_modifier_init(i8*, i32, i32, i32, i8*)
 
 ; 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)
+