https://github.com/alexey-bataev requested changes to this pull request.
Several blockers and design concerns; not ready to land.
1. Tuple-like bindings crash the compiler. `EmitOMPCapturedBindingLValue` ends
in `llvm_unreachable` when `BD->getBinding()->IgnoreImplicit()` is neither
`MemberExpr` nor `ArraySubscriptExpr`, which is exactly the tuple-like case
(`std::pair`, `std::tuple`, `std::array`). Release notes claim this case 'will
produce a compilation error' but no diagnostic is emitted. Restore the
Sema-level rejection for tuple-like bindings (or implement them) and add a
regression test, otherwise any `auto [a,b] = std::pair{...}` followed by an
OpenMP region ICEs.
2. CodeGen is overly hand-rolled. The new `EmitOMPCapturedBindingLValue`
constructs a synthetic `DeclRefExpr` only to call `EmitLValue` and then patches
up the address element type. The standard `VarDecl` branch in
`EmitDeclRefLValue` already handles all the relevant pieces (LocalDeclMap,
reference unwrapping via `EmitLoadOfReferenceLValue`, captured-stmt fallback,
nontemporal SIMD annotation). The new function silently drops the nontemporal
annotation. Reuse the standard path or make the new one consistent with it.
3. SemaStmt.cpp dedup logic and manual reference field. Dropping captures from
`RSI->Captures` for duplicate `DecompositionDecl` ends up with a `CapturedStmt`
whose number of captures no longer matches the original tracked bindings.
Confirm template instantiation, AST dumpers, and source-rewriter clients still
walk this consistently. Also, the manually-constructed `LValueReferenceType`
field ignores `Cap.isReferenceCapture()` and assumes by-ref capture; this
assumption needs an assert at minimum.
4. setOpenMPCaptureKind is now called with the `DecompositionDecl`. DSAs are
tracked per-binding (the user-visible names in `firstprivate(a)` etc.), not
per-decomposed-decl, so this lookup will miss explicit DSAs.
5. Test coverage. No test exercises tuple-like bindings (would crash today),
`target`/`target parallel for`, explicit DSA clauses on bindings,
combined/composite directives, `task`, template instantiation, or static
structured bindings.
6. Release notes wording is wrong. 'C++17 structured bindings in OpenMP
regions' suggests the feature is structured bindings themselves, but those have
always worked. The new feature is 'capturing structured bindings' (a C++20
extension, warned in C++17). Also fix the misleading 'will produce a
compilation error' since the code currently crashes via `llvm_unreachable`.
Inline comments below.
https://github.com/llvm/llvm-project/pull/190832
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
