[Flang][OpenMP] Disable lowering of omp.simd reductions in composites

[skatrak]

Currently, the omp.simd operation is ignored during MLIR to LLVM IR translation when it takes part in a composite construct. One consequence of this limitation is that any entry block arguments defined by that operation will trigger a compiler crash if they are used anywhere, as they are not bound to an LLVM IR value.

A previous PR introducing support for the reduction clause resulted in the creation and use of entry block arguments attached to the omp.simd operation, causing compiler crashes on 'do simd reduction(...)' constructs.

This patch disables Flang lowering of simd reductions in 'do simd' constructs to avoid triggering these errors while translation to LLVM IR is still incomplete.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

Author: Sergio Afonso (skatrak)

Changes

Currently, the omp.simd operation is ignored during MLIR to LLVM IR translation when it takes part in a composite construct. One consequence of this limitation is that any entry block arguments defined by that operation will trigger a compiler crash if they are used anywhere, as they are not bound to an LLVM IR value.

A previous PR introducing support for the reduction clause resulted in the creation and use of entry block arguments attached to the omp.simd operation, causing compiler crashes on 'do simd reduction(...)' constructs.

This patch disables Flang lowering of simd reductions in 'do simd' constructs to avoid triggering these errors while translation to LLVM IR is still incomplete.

---

Full diff: https://github.com/llvm/llvm-project/pull/112686.diff

1 Files Affected:

• (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+14-6)

diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index cf469003b7298d..52a077cd5a797a 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -2209,6 +2209,12 @@ static void genCompositeDistributeParallelDoSimd(
   genSimdClauses(converter, semaCtx, simdItem->clauses, loc, simdClauseOps,
                  simdReductionSyms);
 
+  // TODO: Remove this after omp.simd reductions on composite constructs are
+  // supported.
+  simdClauseOps.reductionVars.clear();
+  simdClauseOps.reductionByref.clear();
+  simdClauseOps.reductionSyms.clear();
+
   mlir::omp::LoopNestOperands loopNestClauseOps;
   llvm::SmallVector<const semantics::Symbol *> iv;
   genLoopNestClauses(converter, semaCtx, eval, simdItem->clauses, loc,
@@ -2230,9 +2236,7 @@ static void genCompositeDistributeParallelDoSimd(
   wsloopOp.setComposite(/*val=*/true);
 
   EntryBlockArgs simdArgs;
-  // TODO: Add private syms and vars.
-  simdArgs.reduction.syms = simdReductionSyms;
-  simdArgs.reduction.vars = simdClauseOps.reductionVars;
+  // TODO: Add private and reduction syms and vars.
   auto simdOp =
       genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);
   simdOp.setComposite(/*val=*/true);
@@ -2325,6 +2329,12 @@ static void genCompositeDoSimd(lower::AbstractConverter &converter,
   genSimdClauses(converter, semaCtx, simdItem->clauses, loc, simdClauseOps,
                  simdReductionSyms);
 
+  // TODO: Remove this after omp.simd reductions on composite constructs are
+  // supported.
+  simdClauseOps.reductionVars.clear();
+  simdClauseOps.reductionByref.clear();
+  simdClauseOps.reductionSyms.clear();
+
   // TODO: Support delayed privatization.
   DataSharingProcessor dsp(converter, semaCtx, simdItem->clauses, eval,
                            /*shouldCollectPreDeterminedSymbols=*/true,
@@ -2348,9 +2358,7 @@ static void genCompositeDoSimd(lower::AbstractConverter &converter,
   wsloopOp.setComposite(/*val=*/true);
 
   EntryBlockArgs simdArgs;
-  // TODO: Add private syms and vars.
-  simdArgs.reduction.syms = simdReductionSyms;
-  simdArgs.reduction.vars = simdClauseOps.reductionVars;
+  // TODO: Add private and reduction syms and vars.
   auto simdOp =
       genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);
   simdOp.setComposite(/*val=*/true);

[kiranchandramohan]

How will this manifest now? Will it be a TODO error? Can you add a quick test?

[kiranchandramohan]

How will this manifest now? Will it be a TODO error? Can you add a quick test?

I see that we are clearing the reduction vars and syms, so that there is no further reduction processing. Would it be better to have a hard TODO to emit an error if there is a reduction in a simd that is part of composite construct?
Note: I dont want to block you if you really need it this way to make progress.

[skatrak]

Sorry Kiran for the delay responding to your comments. Basically what I did here was to revert to the previous behavior for composite constructs, which was to not add it to the operation (here I added it to the builder of omp.simd), while keeping it for standalone simd.

I think we wouldn't want to trigger a TODO for this because the spec allows us to actually ignore simd constructs (i.e. use always a vector size of 1). That's why the MLIR to LLVM IR lowering can just ignore these nested omp.simd wrappers. This patch is sort of a short term solution to prevent crashes in that pass, but I think we should probably do something in MLIR to LLVM IR translation to prevent references to block arguments of an ignored omp.simd wrapper from causing a null pointer dereference until eventually we implement proper translation of the composite SIMD information.

I have an idea on how to do that, but I'd need some time to get to implementing it, so I created this so that this Fujitsu test suite regression is addressed quickly.

[kiranchandramohan]

LG.