Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][CodeGen] Ensure consistent mustprogress attribute emission across all paths #71452

Conversation

antoniofrighetto
Copy link
Contributor

Emission of mustprogress attribute was previously occuring only when entering EmitFunctionBody. Other paths for function body generation may lack the attribute, potentially leading to suboptimal optimizations later in the pipeline. This has been addressed by deferring the attribute emission after the function body has been generated across all the paths.

@antoniofrighetto antoniofrighetto force-pushed the feature/clang-improve-mustprogress branch from f4a92a6 to e02ce12 Compare November 8, 2023 15:16
@antoniofrighetto antoniofrighetto marked this pull request as ready for review November 8, 2023 15:17
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen clang:openmp OpenMP related changes to Clang labels Nov 8, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2023

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Antonio Frighetto (antoniofrighetto)

Changes

Emission of mustprogress attribute was previously occuring only when entering EmitFunctionBody. Other paths for function body generation may lack the attribute, potentially leading to suboptimal optimizations later in the pipeline. This has been addressed by deferring the attribute emission after the function body has been generated across all the paths.


Patch is 75.87 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/71452.diff

16 Files Affected:

  • (modified) clang/lib/CodeGen/CodeGenFunction.cpp (+5-5)
  • (modified) clang/test/CXX/special/class.dtor/p3-0x.cpp (+1-1)
  • (modified) clang/test/CodeGen/fp-floatcontrol-stack.cpp (+5-5)
  • (modified) clang/test/CodeGen/no-builtin.cpp (+4-4)
  • (modified) clang/test/CodeGenCXX/apple-kext.cpp (+2-2)
  • (modified) clang/test/OpenMP/assumes_codegen.cpp (+17-19)
  • (modified) clang/test/OpenMP/for_firstprivate_codegen.cpp (+5-5)
  • (modified) clang/test/OpenMP/for_lastprivate_codegen.cpp (+15-15)
  • (modified) clang/test/OpenMP/for_linear_codegen.cpp (+10-10)
  • (modified) clang/test/OpenMP/parallel_firstprivate_codegen.cpp (+20-20)
  • (modified) clang/test/OpenMP/parallel_private_codegen.cpp (+10-10)
  • (modified) clang/test/OpenMP/parallel_reduction_codegen.cpp (+47-47)
  • (modified) clang/test/OpenMP/sections_firstprivate_codegen.cpp (+10-10)
  • (modified) clang/test/OpenMP/single_firstprivate_codegen.cpp (+5-5)
  • (modified) clang/test/utils/update_cc_test_checks/Inputs/basic-cplusplus.cpp.expected (+2-2)
  • (modified) clang/test/utils/update_cc_test_checks/Inputs/explicit-template-instantiation.cpp.expected (+4-4)
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 6a910abcfe21d2f..f4e2dc373be7f24 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1262,11 +1262,6 @@ void CodeGenFunction::EmitFunctionBody(const Stmt *Body) {
     EmitCompoundStmtWithoutScope(*S);
   else
     EmitStmt(Body);
-
-  // This is checked after emitting the function body so we know if there
-  // are any permitted infinite loops.
-  if (checkIfFunctionMustProgress())
-    CurFn->addFnAttr(llvm::Attribute::MustProgress);
 }
 
 /// When instrumenting to collect profile data, the counts for some blocks
@@ -1482,6 +1477,11 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
   } else
     llvm_unreachable("no definition for emitted function");
 
+  // This is checked after emitting the function body so we know if there
+  // are any permitted infinite loops.
+  if (checkIfFunctionMustProgress())
+    CurFn->addFnAttr(llvm::Attribute::MustProgress);
+
   // C++11 [stmt.return]p2:
   //   Flowing off the end of a function [...] results in undefined behavior in
   //   a value-returning function.
diff --git a/clang/test/CXX/special/class.dtor/p3-0x.cpp b/clang/test/CXX/special/class.dtor/p3-0x.cpp
index f6a64260e0df531..857bdca557fdc4b 100644
--- a/clang/test/CXX/special/class.dtor/p3-0x.cpp
+++ b/clang/test/CXX/special/class.dtor/p3-0x.cpp
@@ -176,4 +176,4 @@ struct TVC : VX
 template <typename T>
 TVC<T>::~TVC() {}
 
-// CHECK: attributes [[ATTRGRP]] = { noinline nounwind{{.*}} }
+// CHECK: attributes [[ATTRGRP]] = { mustprogress noinline nounwind{{.*}} }
diff --git a/clang/test/CodeGen/fp-floatcontrol-stack.cpp b/clang/test/CodeGen/fp-floatcontrol-stack.cpp
index 7357a42838c2d72..090da25d21207d8 100644
--- a/clang/test/CodeGen/fp-floatcontrol-stack.cpp
+++ b/clang/test/CodeGen/fp-floatcontrol-stack.cpp
@@ -224,7 +224,7 @@ float fun_default FUN(1)
 #endif
                                         float y();
 // CHECK-DDEFAULT: Function Attrs: mustprogress noinline nounwind optnone{{$$}}
-// CHECK-DEBSTRICT: Function Attrs: noinline nounwind optnone strictfp{{$$}}
+// CHECK-DEBSTRICT: Function Attrs: mustprogress noinline nounwind optnone strictfp{{$$}}
 // CHECK-FAST: Function Attrs: mustprogress noinline nounwind optnone{{$$}}
 // CHECK-NOHONOR: Function Attrs: mustprogress noinline nounwind optnone{{$$}}
 class ON {
@@ -246,10 +246,10 @@ class ON {
 };
 ON on;
 #pragma float_control(except, off)
-// CHECK-DDEFAULT: Function Attrs: noinline nounwind optnone{{$$}}
-// CHECK-DEBSTRICT: Function Attrs: noinline nounwind optnone{{$$}}
-// CHECK-FAST: Function Attrs: noinline nounwind optnone{{$$}}
-// CHECK-NOHONOR: Function Attrs: noinline nounwind optnone{{$$}}
+// CHECK-DDEFAULT: Function Attrs: mustprogress noinline nounwind optnone{{$$}}
+// CHECK-DEBSTRICT: Function Attrs: mustprogress noinline nounwind optnone{{$$}}
+// CHECK-FAST: Function Attrs: mustprogress noinline nounwind optnone{{$$}}
+// CHECK-NOHONOR: Function Attrs: mustprogress noinline nounwind optnone{{$$}}
 class OFF {
   float w = 2 + y() * 7;
 // CHECK-LABEL: define {{.*}} void @_ZN3OFFC2Ev{{.*}}
diff --git a/clang/test/CodeGen/no-builtin.cpp b/clang/test/CodeGen/no-builtin.cpp
index 14bae1fe1a2234a..bfad88e4ec32496 100644
--- a/clang/test/CodeGen/no-builtin.cpp
+++ b/clang/test/CodeGen/no-builtin.cpp
@@ -42,7 +42,7 @@ extern "C" void call_b_foo(B *b) {
 
 // CHECK-LABEL: define{{.*}} void @call_foo_no_mempcy() #3
 extern "C" void call_foo_no_mempcy() {
-  // CHECK: call void @foo_no_mempcy() #7
+  // CHECK: call void @foo_no_mempcy() #6
   foo_no_mempcy(); // call gets annotated with "no-builtin-memcpy"
 }
 
@@ -50,7 +50,7 @@ A::~A() {} // Anchoring A so A::foo() gets generated
 B::~B() {} // Anchoring B so B::foo() gets generated
 
 // CHECK-LABEL: define linkonce_odr noundef i32 @_ZNK1A3fooEv(ptr noundef{{[^,]*}} %this) unnamed_addr #0 comdat align 2
-// CHECK-LABEL: define linkonce_odr noundef i32 @_ZNK1B3fooEv(ptr noundef{{[^,]*}} %this) unnamed_addr #6 comdat align 2
+// CHECK-LABEL: define linkonce_odr noundef i32 @_ZNK1B3fooEv(ptr noundef{{[^,]*}} %this) unnamed_addr #5 comdat align 2
 
 // CHECK:     attributes #0 = {{{.*}}"no-builtin-memcpy"{{.*}}}
 // CHECK-NOT: attributes #0 = {{{.*}}"no-builtin-memmove"{{.*}}}
@@ -58,7 +58,7 @@ B::~B() {} // Anchoring B so B::foo() gets generated
 // CHECK:     attributes #1 = {{{.*}}"no-builtins"{{.*}}}
 // CHECK:     attributes #2 = {{{.*}}"no-builtin-memcpy"{{.*}}"no-builtin-memset"{{.*}}}
 // CHECK-NOT: attributes #2 = {{{.*}}"no-builtin-memmove"{{.*}}}
-// CHECK:     attributes #6 = {{{.*}}"no-builtin-memmove"{{.*}}}
+// CHECK:     attributes #6 = {{{.*}}"no-builtin-memcpy"{{.*}}}
 // CHECK-NOT: attributes #5 = {{{.*}}"no-builtin-memcpy"{{.*}}}
 // CHECK-NOT: attributes #5 = {{{.*}}"no-builtin-memset"{{.*}}}
-// CHECK:     attributes #7 = { "no-builtin-memcpy" }
+// CHECK:     attributes #8 = { builtin nounwind }
diff --git a/clang/test/CodeGenCXX/apple-kext.cpp b/clang/test/CodeGenCXX/apple-kext.cpp
index d49ef2a0ccfa1b4..2cba0be7a006af5 100644
--- a/clang/test/CodeGenCXX/apple-kext.cpp
+++ b/clang/test/CodeGenCXX/apple-kext.cpp
@@ -38,5 +38,5 @@ namespace test0 {
 // CHECK:      call void @_ZN5test01AD1Ev(ptr @_ZN5test01aE)
 // CHECK-NEXT: ret void
 
-// CHECK: attributes #[[ATTR0]] = { alwaysinline nounwind {{.*}} }
-// CHECK: attributes #[[ATTR1]] = { noinline nounwind {{.*}} }
+// CHECK: attributes #[[ATTR0]] = { alwaysinline mustprogress nounwind {{.*}} }
+// CHECK: attributes #[[ATTR1]] = { mustprogress noinline nounwind {{.*}} }
diff --git a/clang/test/OpenMP/assumes_codegen.cpp b/clang/test/OpenMP/assumes_codegen.cpp
index e47b8ff5b7af54e..6a5871c303aade4 100644
--- a/clang/test/OpenMP/assumes_codegen.cpp
+++ b/clang/test/OpenMP/assumes_codegen.cpp
@@ -115,29 +115,29 @@ int lambda_outer() {
 // CHECK: define{{.*}} void @_Z3barv()
 // CHECK-SAME: [[attr1:#[0-9]]]
 // CHECK:   call{{.*}} @_ZN3BARC1Ev(ptr{{.*}} %b)
-// CHECK-SAME: [[attr9:#[0-9]]]
+// CHECK-SAME: [[attr8:#[0-9]]]
 // CHECK: define{{.*}} void @_ZN3BARC1Ev(ptr{{.*}} %this)
-// CHECK-SAME: [[attr2:#[0-9]]]
+// CHECK-SAME: [[attr1:#[0-9]]]
 // CHECK:   call{{.*}} @_ZN3BARC2Ev(ptr{{.*}} %this1)
-// CHECK-SAME: [[attr9]]
+// CHECK-SAME: [[attr8]]
 // CHECK: define{{.*}} void @_ZN3BARC2Ev(ptr{{.*}} %this)
-// CHECK-SAME: [[attr3:#[0-9]]]
+// CHECK-SAME: [[attr2:#[0-9]]]
 // CHECK: define{{.*}} void @_Z3bazv()
-// CHECK-SAME: [[attr4:#[0-9]]]
+// CHECK-SAME: [[attr3:#[0-9]]]
 // CHECK:   call{{.*}} @_ZN3BAZIfEC1Ev(ptr{{.*}} %b)
-// CHECK-SAME: [[attr10:#[0-9]]]
+// CHECK-SAME: [[attr9:#[0-9]]]
 // CHECK: define{{.*}} void @_ZN3BAZIfEC1Ev(ptr{{.*}} %this)
-// CHECK-SAME: [[attr5:#[0-9]]]
+// CHECK-SAME: [[attr4:#[0-9]]]
 // CHECK:   call{{.*}} @_ZN3BAZIfEC2Ev(ptr{{.*}} %this1)
-// CHECK-SAME: [[attr10]]
+// CHECK-SAME: [[attr9]]
 // CHECK: define{{.*}} void @_ZN3BAZIfEC2Ev(ptr{{.*}} %this)
-// CHECK-SAME: [[attr6:#[0-9]]]
+// CHECK-SAME: [[attr5:#[0-9]]]
 // CHECK: define{{.*}} i32 @_Z12lambda_outerv()
-// CHECK-SAME: [[attr7:#[0-9]]]
+// CHECK-SAME: [[attr6:#[0-9]]]
 // CHECK: call{{.*}} @"_ZZ12lambda_outervENK3$_0clEv"
-// CHECK-SAME: [[attr11:#[0-9]]]
+// CHECK-SAME: [[attr10:#[0-9]]]
 // CHECK: define{{.*}} i32 @"_ZZ12lambda_outervENK3$_0clEv"(ptr{{.*}} %this)
-// CHECK-SAME: [[attr8:#[0-9]]]
+// CHECK-SAME: [[attr7:#[0-9]]]
 
 // CHECK:     attributes [[attr0]]
 // CHECK-SAME:  "llvm.assume"="omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
@@ -146,20 +146,18 @@ int lambda_outer() {
 // CHECK:     attributes [[attr2]]
 // CHECK-SAME:  "llvm.assume"="ompx_range_bar_only,ompx_range_bar_only_2,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
 // CHECK:     attributes [[attr3]]
-// CHECK-SAME:  "llvm.assume"="ompx_range_bar_only,ompx_range_bar_only_2,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
-// CHECK:     attributes [[attr4]]
 // CHECK-SAME:  "llvm.assume"="ompx_1234,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp,ompx_1234,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
+// CHECK:     attributes [[attr4]]
+// CHECK-SAME:  "llvm.assume"="ompx_1234,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
 // CHECK:     attributes [[attr5]]
 // CHECK-SAME:  "llvm.assume"="ompx_1234,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
 // CHECK:     attributes [[attr6]]
-// CHECK-SAME:  "llvm.assume"="ompx_1234,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
+// CHECK-SAME:  "llvm.assume"="ompx_lambda_assumption,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
 // CHECK:     attributes [[attr7]]
 // CHECK-SAME:  "llvm.assume"="ompx_lambda_assumption,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
 // CHECK:     attributes [[attr8]]
-// CHECK-SAME:  "llvm.assume"="ompx_lambda_assumption,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
-// CHECK:     attributes [[attr9]]
 // CHECK-SAME:  "llvm.assume"="ompx_range_bar_only,ompx_range_bar_only_2,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
-// CHECK:     attributes [[attr10]]
+// CHECK:     attributes [[attr9]]
 // CHECK-SAME:  "llvm.assume"="ompx_1234,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
-// CHECK:     attributes [[attr11]]
+// CHECK:     attributes [[attr10]]
 // CHECK-SAME:  "llvm.assume"="ompx_lambda_assumption,omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses,omp_no_openmp"
diff --git a/clang/test/OpenMP/for_firstprivate_codegen.cpp b/clang/test/OpenMP/for_firstprivate_codegen.cpp
index b2c39cf4eb48b32..79f76bd0f4482a1 100644
--- a/clang/test/OpenMP/for_firstprivate_codegen.cpp
+++ b/clang/test/OpenMP/for_firstprivate_codegen.cpp
@@ -412,7 +412,7 @@ int main() {
 //
 //
 // CHECK1-LABEL: define {{[^@]+}}@_Z5tmainIiET_v
-// CHECK1-SAME: () #[[ATTR6:[0-9]+]] {
+// CHECK1-SAME: () #[[ATTR1]] {
 // CHECK1-NEXT:  entry:
 // CHECK1-NEXT:    [[RETVAL:%.*]] = alloca i32, align 4
 // CHECK1-NEXT:    [[TEST:%.*]] = alloca [[STRUCT_S_0:%.*]], align 4
@@ -514,7 +514,7 @@ int main() {
 //
 //
 // CHECK1-LABEL: define {{[^@]+}}@_Z5tmainIiET_v.omp_outlined
-// CHECK1-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[T_VAR:%.*]], ptr noundef nonnull align 4 dereferenceable(8) [[VEC:%.*]], ptr noundef nonnull align 4 dereferenceable(8) [[S_ARR:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[VAR:%.*]]) #[[ATTR7:[0-9]+]] {
+// CHECK1-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[T_VAR:%.*]], ptr noundef nonnull align 4 dereferenceable(8) [[VEC:%.*]], ptr noundef nonnull align 4 dereferenceable(8) [[S_ARR:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[VAR:%.*]]) #[[ATTR6:[0-9]+]] {
 // CHECK1-NEXT:  entry:
 // CHECK1-NEXT:    [[DOTGLOBAL_TID__ADDR:%.*]] = alloca ptr, align 8
 // CHECK1-NEXT:    [[DOTBOUND_TID__ADDR:%.*]] = alloca ptr, align 8
@@ -1013,7 +1013,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@__main_block_invoke
-// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR1]] {
+// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR4:[0-9]+]] {
 // CHECK4-NEXT:  entry:
 // CHECK4-NEXT:    [[DOTBLOCK_DESCRIPTOR_ADDR:%.*]] = alloca ptr, align 8
 // CHECK4-NEXT:    [[BLOCK_ADDR:%.*]] = alloca ptr, align 8
@@ -1024,7 +1024,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@__main_block_invoke.omp_outlined
-// CHECK4-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[SIVAR:%.*]]) #[[ATTR4:[0-9]+]] {
+// CHECK4-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[SIVAR:%.*]]) #[[ATTR5:[0-9]+]] {
 // CHECK4-NEXT:  entry:
 // CHECK4-NEXT:    [[DOTGLOBAL_TID__ADDR:%.*]] = alloca ptr, align 8
 // CHECK4-NEXT:    [[DOTBOUND_TID__ADDR:%.*]] = alloca ptr, align 8
@@ -1129,7 +1129,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@var_block_invoke
-// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR1]] {
+// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR4]] {
 // CHECK4-NEXT:  entry:
 // CHECK4-NEXT:    [[DOTBLOCK_DESCRIPTOR_ADDR:%.*]] = alloca ptr, align 8
 // CHECK4-NEXT:    [[BLOCK_ADDR:%.*]] = alloca ptr, align 8
diff --git a/clang/test/OpenMP/for_lastprivate_codegen.cpp b/clang/test/OpenMP/for_lastprivate_codegen.cpp
index e0a11b6b87ee64a..c7ef60afcfc0329 100644
--- a/clang/test/OpenMP/for_lastprivate_codegen.cpp
+++ b/clang/test/OpenMP/for_lastprivate_codegen.cpp
@@ -835,7 +835,7 @@ int main() {
 //
 //
 // CHECK1-LABEL: define {{[^@]+}}@_Z5tmainIiET_v
-// CHECK1-SAME: () #[[ATTR7:[0-9]+]] {
+// CHECK1-SAME: () #[[ATTR1]] {
 // CHECK1-NEXT:  entry:
 // CHECK1-NEXT:    [[RETVAL:%.*]] = alloca i32, align 4
 // CHECK1-NEXT:    [[TEST:%.*]] = alloca [[STRUCT_S_0:%.*]], align 4
@@ -1636,7 +1636,7 @@ int main() {
 //
 //
 // CHECK3-LABEL: define {{[^@]+}}@_ZN2SSC2ERi.omp_outlined
-// CHECK3-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]]) #[[ATTR3:[0-9]+]] {
+// CHECK3-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]]) #[[ATTR2:[0-9]+]] {
 // CHECK3-NEXT:  entry:
 // CHECK3-NEXT:    [[DOTGLOBAL_TID__ADDR:%.*]] = alloca ptr, align 8
 // CHECK3-NEXT:    [[DOTBOUND_TID__ADDR:%.*]] = alloca ptr, align 8
@@ -1773,7 +1773,7 @@ int main() {
 //
 //
 // CHECK3-LABEL: define {{[^@]+}}@_ZZN2SSC1ERiENKUlvE_clEv
-// CHECK3-SAME: (ptr noundef nonnull align 8 dereferenceable(32) [[THIS:%.*]]) #[[ATTR2:[0-9]+]] align 2 {
+// CHECK3-SAME: (ptr noundef nonnull align 8 dereferenceable(32) [[THIS:%.*]]) #[[ATTR1]] align 2 {
 // CHECK3-NEXT:  entry:
 // CHECK3-NEXT:    [[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK3-NEXT:    store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
@@ -1806,7 +1806,7 @@ int main() {
 //
 //
 // CHECK3-LABEL: define {{[^@]+}}@_ZZN2SSC1ERiENKUlvE0_clEv
-// CHECK3-SAME: (ptr noundef nonnull align 8 dereferenceable(16) [[THIS:%.*]]) #[[ATTR2]] align 2 {
+// CHECK3-SAME: (ptr noundef nonnull align 8 dereferenceable(16) [[THIS:%.*]]) #[[ATTR1]] align 2 {
 // CHECK3-NEXT:  entry:
 // CHECK3-NEXT:    [[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK3-NEXT:    store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
@@ -1845,7 +1845,7 @@ int main() {
 //
 //
 // CHECK3-LABEL: define {{[^@]+}}@_ZZN2SSC1ERiENKUlvE_clEv.omp_outlined
-// CHECK3-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[A:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[B:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[C:%.*]]) #[[ATTR3]] {
+// CHECK3-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[A:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[B:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[C:%.*]]) #[[ATTR2]] {
 // CHECK3-NEXT:  entry:
 // CHECK3-NEXT:    [[DOTGLOBAL_TID__ADDR:%.*]] = alloca ptr, align 8
 // CHECK3-NEXT:    [[DOTBOUND_TID__ADDR:%.*]] = alloca ptr, align 8
@@ -1962,7 +1962,7 @@ int main() {
 //
 //
 // CHECK3-LABEL: define {{[^@]+}}@_ZZN2SSC1ERiENKUlvE0_clEv.omp_outlined
-// CHECK3-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[A:%.*]]) #[[ATTR3]] {
+// CHECK3-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[A:%.*]]) #[[ATTR2]] {
 // CHECK3-NEXT:  entry:
 // CHECK3-NEXT:    [[DOTGLOBAL_TID__ADDR:%.*]] = alloca ptr, align 8
 // CHECK3-NEXT:    [[DOTBOUND_TID__ADDR:%.*]] = alloca ptr, align 8
@@ -2105,7 +2105,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@__main_block_invoke
-// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR1]] {
+// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR2:[0-9]+]] {
 // CHECK4-NEXT:  entry:
 // CHECK4-NEXT:    [[DOTBLOCK_DESCRIPTOR_ADDR:%.*]] = alloca ptr, align 8
 // CHECK4-NEXT:    [[BLOCK_ADDR:%.*]] = alloca ptr, align 8
@@ -2116,7 +2116,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@__main_block_invoke.omp_outlined
-// CHECK4-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[SIVAR:%.*]]) #[[ATTR2:[0-9]+]] {
+// CHECK4-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[SIVAR:%.*]]) #[[ATTR3:[0-9]+]] {
 // CHECK4-NEXT:  entry:
 // CHECK4-NEXT:    [[DOTGLOBAL_TID__ADDR:%.*]] = alloca ptr, align 8
 // CHECK4-NEXT:    [[DOTBOUND_TID__ADDR:%.*]] = alloca ptr, align 8
@@ -2232,7 +2232,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@g1_block_invoke
-// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR1]] {
+// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR2]] {
 // CHECK4-NEXT:  entry:
 // CHECK4-NEXT:    [[DOTBLOCK_DESCRIPTOR_ADDR:%.*]] = alloca ptr, align 8
 // CHECK4-NEXT:    [[BLOCK_ADDR:%.*]] = alloca ptr, align 8
@@ -2345,7 +2345,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@_ZN2SSC2ERi.omp_outlined
-// CHECK4-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]]) #[[ATTR2]] {
+// CHECK4-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]]) #[[ATTR3]] {
 // CHECK4-NEXT:  entry:
 // CHECK4-NEXT:    [[DOTGLOBAL_TID__ADDR:%.*]] = alloca ptr, align 8
 // CHECK4-NEXT:    [[DOTBOUND_TID__ADDR:%.*]] = alloca ptr, align 8
@@ -2495,7 +2495,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@g1_block_invoke_2
-// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR1]] {
+// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR2]] {
 // CHECK4-NEXT:  entry:
 // CHECK4-NEXT:    [[DOTBLOCK_DESCRIPTOR_ADDR:%.*]] = alloca ptr, align 8
 // CHECK4-NEXT:    [[BLOCK_ADDR:%.*]] = alloca ptr, align 8
@@ -2527,7 +2527,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@g1_block_invoke_2.omp_outlined
-// CHECK4-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[A:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[B:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[C:%.*]]) #[[ATTR2]] {
+// CHECK4-SAME: (ptr noalias noundef [[DOTGLOBAL_TID_:%.*]], ptr noalias noundef [[DOTBOUND_TID_:%.*]], ptr noundef [[THIS:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[A:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[B:%.*]], ptr noundef nonnull align 4 dereferenceable(4) [[C:%.*]]) #[[ATTR3]] {
 // CHECK4-NEXT:  entry:
 // CHECK4-NEXT:    [[DOTGLOBAL_TID__ADDR:%.*]] = alloca ptr, align 8
 // CHECK4-NEXT:    [[DOTBOUND_TID__ADDR:%.*]] = alloca ptr, align 8
@@ -2644,7 +2644,7 @@ int main() {
 //
 //
 // CHECK4-LABEL: define {{[^@]+}}@___ZN2SSC2ERi_block_invoke
-// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]]) #[[ATTR1]] {
+// CHECK4-SAME: (ptr noundef [[DOTBLOCK_DESCRIPTOR:%.*]])...
[truncated]

@antoniofrighetto
Copy link
Contributor Author

I wonder if we should have return getLangOpts().CPlusPlus11 || getLangOpts().C11; at line 586:

bool checkIfFunctionMustProgress() {
if (CGM.getCodeGenOpts().getFiniteLoops() ==
CodeGenOptions::FiniteLoopsKind::Never)
return false;
// C++11 and later guarantees that a thread eventually will do one of the
// following (C++11 [intro.multithread]p24 and C++17 [intro.progress]p1):
// - terminate,
// - make a call to a library I/O function,
// - perform an access through a volatile glvalue, or
// - perform a synchronization operation or an atomic operation.
//
// Hence each function is 'mustprogress' in C++11 or later.
return getLangOpts().CPlusPlus11;
}

But I couldn't find anything strictly related in C11 7.26 and 7.31.15.

@efriedma-quic
Copy link
Collaborator

The C equivalent to the C++11 rule is in 6.8.5, which says "An iteration statement may be assumed by the implementation to terminate if [...]". There's no general rule that a program has to make progress if there isn't a loop involved.

@@ -1482,6 +1477,11 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
} else
llvm_unreachable("no definition for emitted function");

// This is checked after emitting the function body so we know if there
// are any permitted infinite loops.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment actually accurate? I think the relevant code changed in 6c31295

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment was added in ac73b73. Perhaps something along these lines could work better?

// This is checked after emitting the function body, so as to ensure
// that the function adheres to the forward progress guarantee,
// which is required by certain optimizations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fundamentally, I'm not sure there's any reason we need to do this after emitting the function body. When the comment was written, it was referring to the FnIsMustProgress bit in CodeGenFunction, but that was removed.

Copy link
Contributor Author

@antoniofrighetto antoniofrighetto Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we fail to with some optimizations (e.g., #69833) due to a few non-mustprogress function, when they should likely not be. Doing this in GenerateCode after emitting function body looks reasonable to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean, there isn't a reason to specifically do it after emitting the function body, as opposed to before we emit it (when we compute most other attributes).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this, I assumed that this was the only way to know, as per comment says, if there are any permitted infinite loops.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at the implementation of checkIfFunctionMustProgress(), it's pretty easy to verify that it doesn't actually look at the body of the function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anticipated before function body generation, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirm this closes #69833.

@antoniofrighetto antoniofrighetto force-pushed the feature/clang-improve-mustprogress branch from e02ce12 to f80a427 Compare November 10, 2023 17:01
Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Emission of `mustprogress` attribute previously occurred only within
`EmitFunctionBody`, after generating the function body. Other routines
for function body creation may lack the attribute, potentially leading
to suboptimal optimizations later in the pipeline. Attribute emission
is now anticipated prior to generating the function body.

Fixes: llvm#69833.
@antoniofrighetto antoniofrighetto force-pushed the feature/clang-improve-mustprogress branch from f80a427 to 970bf07 Compare November 11, 2023 08:44
@antoniofrighetto antoniofrighetto merged commit 970bf07 into llvm:main Nov 11, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants