[flang-commits] [flang] [mlir] [Flang][OpenMP] Support conditional lastprivate on host (PR #200086)

[Tom Eccles via flang-commits]

tblah wrote:

Thanks for following up. I am still finding serious correctness issues in this
patch after several review rounds. Previous rounds caught unsupported constructs
being silently accepted, missing type restrictions, handling problems for some
types, copyback correctness issues, and other semantic problems.

My latest findings are:

- interaction with firstprivate: the loop body reads the variable from the
  zero-initialized struct created for conditional lastprivate, not from the
  firstprivate copy.
- `distribute parallel do simd lastprivate(conditional:)` does not hit the SIMD
  TODO and is silently lowered.
- missing test coverage under `mlir/test/Target/...` for the
  `OpenMPToLLVMIRTranslation.cpp` change.

This is taking a disproportionate amount of reviewer time to construct tests,
verify the behavior, and communicate the results in a useful way.

For the next revision, please add a substantial test matrix covering the
supported and unsupported OpenMP constructs, the relevant supported and
unsupported list item categories, orphaned and non-orphaned forms, diagnostics
or TODOs for unsupported behavior, interactions with other clauses that
influence symbol mapping, and multiple conditional lastprivate variables.

These tests should each be minimal lit tests local to the layer being tested.
For example, cases that should be caught by Flang semantics should be tested
alongside the other Flang semantic checks. Changes to MLIR-to-LLVMIR translation
should have minimal lit coverage alongside the other tests for that translation
unit.

You do not have to support every type or construct combination in this PR. But
at each interface level, both Fortran and MLIR, every accepted case should
either lower correctly or fail gracefully with a diagnostic/TODO. There should
be no silent miscompilations, assertion failures, or compiler crashes.

Once the PR is more strongly tested, I will take another look.


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