[flang-commits] [flang] [flang][OpenMP] Lower multi-type user-defined declare reduction (PR #207494)

[Tom Eccles via flang-commits]

================
@@ -819,52 +825,87 @@ bool ReductionProcessor::processReductionArguments(
                                  omp::clause::ReductionOperatorList>) {
       const Fortran::lower::omp::clause::ReductionOperator &redOperator =
           redOperatorList.front();
+      // Name user-defined reduction ops from the canonical element type, not
+      // the raw lowered variable type. An allocatable/pointer variable lowers
+      // to a boxed reference (!fir.ref<!fir.box<!fir.heap<!fir.char<1>>>>), but
+      // the directive names its op from the declared element type
+      // (!fir.char<1>); getReductionName unwraps only one reference level, so
+      // without stripping the ref/box/heap/pointer wrappers the by-ref suffix
+      // diverges and a valid allocatable-character reduction is wrongly
+      // rejected. Keep any array fir::SequenceType. Only naming and the type
+      // check use namingType; redType stays intact for op binding.
+      mlir::Type namingType =
+          fir::unwrapPassByRefType(fir::unwrapRefType(redType));
+      // An allocatable/pointer reduction of a trivial element type is a case
+      // the directive does not materialize: it emits the scalar by-value op,
+      // and binding the boxed by-ref operand to it is invalid IR. In the
+      // default mode the by-ref name suffix diverges (boxed clause by-ref,
+      // scalar op by-value), the lookup misses, and it is a clean TODO. Under
+      // -mmlir
+      // --force-byref-reduction both sides are forced by-ref, the names match,
+      // and the element-only type check (i32 == i32) would bind the box to the
+      // scalar op. Guard on the inherent triviality of the element type, not
+      // doReductionByRef (which the flag forces), so it is a clean TODO in
+      // every mode; boxed character and derived reductions (genuinely by-ref)
+      // still bind. Deferred to flang PR #186765.
+      const bool isBoxedTrivialReduction =
+          mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(redType)) &&
+          fir::isa_trivial(namingType);
       if (const auto &redDefinedOp =
               std::get_if<omp::clause::DefinedOperator>(&redOperator.u)) {
         if (const auto *definedOpName =
                 std::get_if<omp::clause::DefinedOperator::DefinedOpName>(
                     &redDefinedOp->u)) {
           // User-defined operator reduction (e.g. reduction(.myop.:x)). Resolve
-          // the use-site operator to its reduction symbol, which semantics
-          // names "op<spelling>" (MangleDefinedOperator in resolve-names), in
-          // the current scope, then reference the omp.declare_reduction op the
-          // directive materialized for it. Only a locally-declared,
-          // single-declaration, single-type reduction whose type the variable
-          // supports is handled here; anything else (imported, renamed, merged,
-          // or multiple declarations/types) is a clean TODO rather than a crash
-          // or a wrong binding.
+          // the use-site operator to the source reduction symbol as the OpenMP
+          // semantic checks do (following USE association, operator renames,
+          // private visibility, and merged generics), then reference the
+          // omp.declare_reduction op named from that symbol's scoped (name,
+          // owner). Reductions in different source modules have distinct owner
+          // scopes, so a same-spelling/same-type operator reduction imported
+          // from two modules yields two distinct ops, each clause binding its
+          // own combiner instead of colliding onto one. The variable's type
+          // selects the matching per-type op name (getScopedUserReductionName
+          // appends the type), so a multiple-declaration/multiple-type operator
+          // is handled (one op per type). An operator with no reduction for the
+          // variable's type, or a variable with no type, is a clean TODO rather
+          // than a crash or a wrong binding.
           const semantics::Symbol *opSym = definedOpName->v.sym();
-          std::string mangledName = "op" + opSym->name().ToString();
-          const semantics::Symbol *redSym =
-              converter.getCurrentScope().FindSymbol(
-                  parser::CharBlock{mangledName});
+          const semantics::DeclTypeSpec *varType =
+              reductionSymbols[idx]->GetUltimate().GetType();
+          // The variable's type disambiguates an operator that carries
+          // reductions for several types (e.g. a generic merged from multiple
+          // single-type modules): the resolver returns only a reduction that
+          // supports varType.
+          const semantics::Symbol *resolvedSym =
+              semantics::omp::FindOperatorUserReductionSymbol(*opSym, varType);
+          // resolvedSym is the found symbol, which may be a USE-associated
+          // wrapper; take its ultimate before reading the reduction details.
           const semantics::Symbol *ultimate =
-              redSym ? &redSym->GetUltimate() : nullptr;
+              resolvedSym ? &resolvedSym->GetUltimate() : nullptr;
           const semantics::UserReductionDetails *userDetails =
               ultimate ? ultimate->detailsIf<semantics::UserReductionDetails>()
                        : nullptr;
-          const semantics::DeclTypeSpec *varType =
-              reductionSymbols[idx]->GetUltimate().GetType();
-          if (!redSym || ultimate != redSym || !userDetails ||
-              userDetails->GetDeclList().size() != 1 ||
-              userDetails->GetTypeList().size() != 1 || !varType ||
-              !userDetails->SupportsType(*varType)) {
+          if (!varType || !resolvedSym || !userDetails) {
             TODO(currentLocation,
                  "OpenMP user-defined operator reduction is not yet supported "
-                 "for imported, renamed, or multiple-declaration/type "
-                 "reductions");
+                 "when the variable has no type or no matching reduction is "
+                 "visible for its type");
----------------
tblah wrote:

I guess this wasn't introduced by your patch but I wonder whether this is a genuine TODO or just a bug in semantic checking? Maybe it is a case for `fir::emitFatalError`?

https://github.com/llvm/llvm-project/pull/207494