https://github.com/dmikushin updated https://github.com/llvm/llvm-project/pull/202292
>From 38dba947bb84cde82364ba06fa2d7630502fff03 Mon Sep 17 00:00:00 2001 From: Dmitry Mikushin <[email protected]> Date: Sun, 7 Jun 2026 00:39:55 +0200 Subject: [PATCH 1/2] [Clang][OpenMP] Fix '*' reduction identity for class types Clang accepts OpenMP built-in reductions over C++ class types such as std::complex<T> as an extension (the OpenMP standard only defines the implicitly-declared reduction identities for arithmetic and, in Clang, for _Complex types; GCC rejects the construct outright). For such class types the private reduction copy was left value-initialized, which yields the *additive* identity (e.g. std::complex(0,0)). For '+' this happens to be correct, but for '*' it is wrong: every thread starts from (0,0), so the product collapses to (0,0) on the host -- even with a single thread, since this is purely a wrong identity element and not a parallel-combine problem. Fix the BO_Mul case in actOnOMPReductionKindClause to also synthesize the integer-literal '1' initializer for C++ record types, mirroring the existing scalar/_Complex path. The converting constructor then builds the multiplicative identity (e.g. std::complex(1) == (1,0)). If the type is not constructible from '1', AddInitializerToDecl diagnoses it and the list item is dropped, so we never silently miscompile (no ICE). BO_Add is intentionally left relying on value-initialization for class types, because there the (0,0) default is the correct additive identity in the common case. Adds a codegen test verifying the private copy is initialized to (1,0) for '*' and (0,0) for '+', a -verify test covering the diagnostic for a class type that is not constructible from '1', and a release note. --- clang/docs/ReleaseNotes.rst | 4 ++ clang/lib/Sema/SemaOpenMP.cpp | 21 ++++++++- .../for_reduction_class_identity_codegen.cpp | 43 +++++++++++++++++++ .../for_reduction_class_identity_messages.cpp | 40 +++++++++++++++++ 4 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 clang/test/OpenMP/for_reduction_class_identity_codegen.cpp create mode 100644 clang/test/OpenMP/for_reduction_class_identity_messages.cpp diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index bf917b3f642bc..25a2e9e59e9b3 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -969,6 +969,10 @@ OpenMP Support ``fallback`` modifier (``fb_nullify`` or ``fb_preserve``) with OpenMP >= 61. - Added support for ``local`` clause with declare_target directive when OpenMP >= 60. +- Fixed the identity element used for ``reduction(* : x)`` over C++ class types + (e.g. ``std::complex``). The private copy is now initialized to the + multiplicative identity instead of being value-initialized, which previously + produced a wrong result (the product collapsed to the additive identity). SYCL Support ------------ diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp index 76b40a5039180..1d11776824976 100644 --- a/clang/lib/Sema/SemaOpenMP.cpp +++ b/clang/lib/Sema/SemaOpenMP.cpp @@ -20911,9 +20911,28 @@ static bool actOnOMPReductionKindClause( Init = S.ActOnIntegerConstant(ELoc, /*Val=*/0).get(); break; case BO_Mul: + // '*' reduction op - initializer is '1'. + // For C++ class types (e.g. std::complex) the OpenMP built-in + // reduction identifiers are an extension: the standard only defines + // identities for arithmetic (and, in Clang, _Complex) types. Without + // an explicit initializer the private copy would be value-initialized, + // which yields the *additive* identity (e.g. std::complex(0,0)) and is + // wrong for multiplication. Initialize from the integer literal '1' + // instead and let the converting constructor build the multiplicative + // identity (e.g. std::complex(1) == (1,0)). If the type is not + // constructible from '1', AddInitializerToDecl below diagnoses it and + // the list item is dropped, so we never silently miscompile. + // Note: BO_Add deliberately keeps relying on value-initialization for + // class types, because there the (0,0) default is the correct additive + // identity for the common case; only '*' needs this special handling. + if (Type->isScalarType() || Type->isAnyComplexType() || + (S.getLangOpts().CPlusPlus && Type->isRecordType())) { + Init = S.ActOnIntegerConstant(ELoc, /*Val=*/1).get(); + } + break; case BO_LAnd: if (Type->isScalarType() || Type->isAnyComplexType()) { - // '*' and '&&' reduction ops - initializer is '1'. + // '&&' reduction ops - initializer is '1'. Init = S.ActOnIntegerConstant(ELoc, /*Val=*/1).get(); } break; diff --git a/clang/test/OpenMP/for_reduction_class_identity_codegen.cpp b/clang/test/OpenMP/for_reduction_class_identity_codegen.cpp new file mode 100644 index 0000000000000..34640f51b31b2 --- /dev/null +++ b/clang/test/OpenMP/for_reduction_class_identity_codegen.cpp @@ -0,0 +1,43 @@ +// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-linux-gnu -emit-llvm %s -o - | FileCheck %s +// expected-no-diagnostics + +struct Complex { + double Re; + double Im; + Complex(double Re = 0.0, double Im = 0.0) : Re(Re), Im(Im) {} + Complex &operator+=(const Complex &RHS) { + Re += RHS.Re; + Im += RHS.Im; + return *this; + } + Complex &operator*=(const Complex &RHS) { + double NewRe = Re * RHS.Re - Im * RHS.Im; + Im = Re * RHS.Im + Im * RHS.Re; + Re = NewRe; + return *this; + } +}; + +void add_reduction(Complex *Data) { + Complex Sum; +#pragma omp parallel for reduction(+ : Sum) + for (int I = 0; I < 4; ++I) + Sum += Data[I]; +} + +void mul_reduction(Complex *Data) { + Complex Product; +#pragma omp parallel for reduction(* : Product) + for (int I = 0; I < 4; ++I) + Product *= Data[I]; +} + +// CHECK-LABEL: define {{.*}}void @_Z13add_reductionP7Complex +// CHECK: call {{.*}} @_ZN7ComplexC1Edd(ptr {{[^,]*}}%{{[^,]*}}, double {{.*}}0.000000e+00, double {{.*}}0.000000e+00) +// CHECK-LABEL: define internal {{.*}}void @_Z13add_reductionP7Complex.omp_outlined +// CHECK: call {{.*}} @_ZN7ComplexC1Edd(ptr {{[^,]*}}%{{[^,]*}}, double {{.*}}0.000000e+00, double {{.*}}0.000000e+00) + +// CHECK-LABEL: define {{.*}}void @_Z13mul_reductionP7Complex +// CHECK: call {{.*}} @_ZN7ComplexC1Edd(ptr {{[^,]*}}%{{[^,]*}}, double {{.*}}0.000000e+00, double {{.*}}0.000000e+00) +// CHECK-LABEL: define internal {{.*}}void @_Z13mul_reductionP7Complex.omp_outlined +// CHECK: call {{.*}} @_ZN7ComplexC1Edd(ptr {{[^,]*}}%{{[^,]*}}, double {{.*}}1.000000e+00, double {{.*}}0.000000e+00) diff --git a/clang/test/OpenMP/for_reduction_class_identity_messages.cpp b/clang/test/OpenMP/for_reduction_class_identity_messages.cpp new file mode 100644 index 0000000000000..7cf75ad641d93 --- /dev/null +++ b/clang/test/OpenMP/for_reduction_class_identity_messages.cpp @@ -0,0 +1,40 @@ +// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-linux-gnu -ferror-limit 100 %s +// RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-linux-gnu -ferror-limit 100 %s + +// Multiplication reduction over a class type that *is* constructible from the +// integer literal '1' (e.g. std::complex-like) is accepted: the multiplicative +// identity is built via the converting constructor. See +// for_reduction_class_identity_codegen.cpp for the value check. +struct WithInt { + double V; + WithInt(int X = 1) : V(X) {} + WithInt &operator*=(const WithInt &RHS) { + V *= RHS.V; + return *this; + } +}; + +void ok(WithInt *Data) { + WithInt Product; +#pragma omp parallel for reduction(* : Product) + for (int I = 0; I < 4; ++I) + Product *= Data[I]; +} + +// Multiplication reduction over a class type that is NOT constructible from '1' +// must be diagnosed rather than silently miscompiled (it previously fell back +// to value-initialization, producing the wrong (additive) identity). +struct NoInt { // expected-note 2 {{candidate constructor}} + double V; + NoInt &operator*=(const NoInt &RHS) { + V *= RHS.V; + return *this; + } +}; + +void bad(NoInt *Data) { + NoInt Product; +#pragma omp parallel for reduction(* : Product) // expected-error {{no viable conversion from 'int' to 'NoInt'}} + for (int I = 0; I < 4; ++I) + Product *= Data[I]; +} >From e23b03edaac22c25c17131a352fa41fea5b96711 Mon Sep 17 00:00:00 2001 From: Dmitry Mikushin <[email protected]> Date: Wed, 10 Jun 2026 16:22:50 +0200 Subject: [PATCH 2/2] Fall back to value-initialization when the type is not constructible from '1' Address review feedback: instead of letting AddInitializerToDecl emit a hard error for class types that are not copy-initializable from the integer literal '1', probe the initialization with an InitializationSequence first and fall back to value-initialization (the pre-existing behavior) when it is not viable. This way a reduction that used to compile keeps compiling, and only types that actually support '1' (e.g. std::complex) get the multiplicative identity. --- clang/lib/Sema/SemaOpenMP.cpp | 25 +++++++++++++------ .../for_reduction_class_identity_messages.cpp | 12 +++++---- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp index 1d11776824976..5dc1e32e7734f 100644 --- a/clang/lib/Sema/SemaOpenMP.cpp +++ b/clang/lib/Sema/SemaOpenMP.cpp @@ -20919,15 +20919,24 @@ static bool actOnOMPReductionKindClause( // which yields the *additive* identity (e.g. std::complex(0,0)) and is // wrong for multiplication. Initialize from the integer literal '1' // instead and let the converting constructor build the multiplicative - // identity (e.g. std::complex(1) == (1,0)). If the type is not - // constructible from '1', AddInitializerToDecl below diagnoses it and - // the list item is dropped, so we never silently miscompile. - // Note: BO_Add deliberately keeps relying on value-initialization for - // class types, because there the (0,0) default is the correct additive - // identity for the common case; only '*' needs this special handling. - if (Type->isScalarType() || Type->isAnyComplexType() || - (S.getLangOpts().CPlusPlus && Type->isRecordType())) { + // identity (e.g. std::complex(1) == (1,0)). + if (Type->isScalarType() || Type->isAnyComplexType()) { Init = S.ActOnIntegerConstant(ELoc, /*Val=*/1).get(); + } else if (S.getLangOpts().CPlusPlus && Type->isRecordType()) { + // Only use '1' when the type is actually copy-initializable from it. + // Otherwise fall back to value-initialization (the previous behavior) + // rather than rejecting the reduction, so a class that used to + // compile keeps compiling. Such a class keeps its (possibly + // incorrect) value-initialized identity, matching the pre-existing + // behavior; BO_Add likewise relies on value-initialization for class + // types. + Expr *One = S.ActOnIntegerConstant(ELoc, /*Val=*/1).get(); + InitializedEntity Entity = + InitializedEntity::InitializeTemporary(Type); + InitializationKind Kind = InitializationKind::CreateCopy(ELoc, ELoc); + InitializationSequence Seq(S, Entity, Kind, One); + if (Seq) + Init = One; } break; case BO_LAnd: diff --git a/clang/test/OpenMP/for_reduction_class_identity_messages.cpp b/clang/test/OpenMP/for_reduction_class_identity_messages.cpp index 7cf75ad641d93..0ef9a2938a033 100644 --- a/clang/test/OpenMP/for_reduction_class_identity_messages.cpp +++ b/clang/test/OpenMP/for_reduction_class_identity_messages.cpp @@ -1,5 +1,6 @@ // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-linux-gnu -ferror-limit 100 %s // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-linux-gnu -ferror-limit 100 %s +// expected-no-diagnostics // Multiplication reduction over a class type that *is* constructible from the // integer literal '1' (e.g. std::complex-like) is accepted: the multiplicative @@ -22,9 +23,10 @@ void ok(WithInt *Data) { } // Multiplication reduction over a class type that is NOT constructible from '1' -// must be diagnosed rather than silently miscompiled (it previously fell back -// to value-initialization, producing the wrong (additive) identity). -struct NoInt { // expected-note 2 {{candidate constructor}} +// must NOT be rejected: we fall back to value-initialization (the pre-existing +// behavior) instead of emitting a hard error, so code that used to compile keeps +// compiling. +struct NoInt { double V; NoInt &operator*=(const NoInt &RHS) { V *= RHS.V; @@ -32,9 +34,9 @@ struct NoInt { // expected-note 2 {{candidate constructor}} } }; -void bad(NoInt *Data) { +void fallback(NoInt *Data) { NoInt Product; -#pragma omp parallel for reduction(* : Product) // expected-error {{no viable conversion from 'int' to 'NoInt'}} +#pragma omp parallel for reduction(* : Product) for (int I = 0; I < 4; ++I) Product *= Data[I]; } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
