[Flang][OpenMP] Remove omp.simd reduction block args

[skatrak]

This patch reverts previous changes to create entry block arguments for reduction variables attached to simd constructs.

This can't currently be done because reduction variables stored in the corresponding clause structure are not added to the omp.simd operation when created, as this is not supported yet. Adding block arguments for non-existent reduction variables results in some tests from the Fujitsu compiler testsuite breaking: https://linaro.atlassian.net/browse/LLVM-1389.

[llvmbot]

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

Author: Sergio Afonso (skatrak)

Changes

This patch reverts previous changes to create entry block arguments for reduction variables attached to simd constructs.

This can't currently be done because reduction variables stored in the corresponding clause structure are not added to the omp.simd operation when created, as this is not supported yet. Adding block arguments for non-existent reduction variables results in some tests from the Fujitsu compiler testsuite breaking: https://linaro.atlassian.net/browse/LLVM-1389.

---

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

1 Files Affected:

• (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+4-12)

diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index b1a10960c8022e..1ce744457160b4 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -2072,9 +2072,7 @@ static void genStandaloneSimd(lower::AbstractConverter &converter,
                      loopNestClauseOps, iv);
 
   EntryBlockArgs simdArgs;
-  // TODO: Add private syms and vars.
-  simdArgs.reduction.syms = simdReductionSyms;
-  simdArgs.reduction.vars = simdClauseOps.reductionVars;
+  // TODO: Add private, reduction syms and vars.
   auto simdOp =
       genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);
 
@@ -2232,9 +2230,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, reduction syms and vars.
   auto simdOp =
       genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);
   simdOp.setComposite(/*val=*/true);
@@ -2291,9 +2287,7 @@ static void genCompositeDistributeSimd(lower::AbstractConverter &converter,
   distributeOp.setComposite(/*val=*/true);
 
   EntryBlockArgs simdArgs;
-  // TODO: Add private syms and vars.
-  simdArgs.reduction.syms = simdReductionSyms;
-  simdArgs.reduction.vars = simdClauseOps.reductionVars;
+  // TODO: Add private, reduction syms and vars.
   auto simdOp =
       genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);
   simdOp.setComposite(/*val=*/true);
@@ -2350,9 +2344,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, reduction syms and vars.
   auto simdOp =
       genWrapperOp<mlir::omp::SimdOp>(converter, loc, simdClauseOps, simdArgs);
   simdOp.setComposite(/*val=*/true);

[tblah]

I'm okay with you merging this to fix the test runner but I have some questions.

Why did adding block arguments break some tests? Do we have a broken index calculation somewhere returning the wrong block arguments?

Why do we get this far with an unsupported clause? Shouldn't it have a TODO?

[skatrak]

I'm okay with you merging this to fix the test runner but I have some questions.

Why did adding block arguments break some tests? Do we have a broken index calculation somewhere returning the wrong block arguments?

If you check the builder for omp.simd based on the operand structure, you see that reduction operands are not being added to the operation. If we added the corresponding block arguments, there would be a mismatch that results in some assertions being triggered while binding them. I'd say index calculation is fine, it's just that it's based on the arguments the operation itself has (0 in this case).

Why do we get this far with an unsupported clause? Shouldn't it have a TODO?

I'm not sure, my guess is to let do simd reduction(...) work by only processing the reduction at the do level. If we triggered a TODO for simd reduction(...), we would hit it in that other case as well. omp.simd with reduction does trigger an error in translation to LLVM IR, so another option is to fully support it in flang lowering and let it complain later in the process (which wouldn't currently trigger for do simd because omp.wsloop lowering skips other nested wrappers).

[skatrak]

I'll merge this for now, but we can keep discussing to figure out a longer term solution.