Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P1673: LWG review 2023/11/06 (Kona) #425

Open
19 of 22 tasks
mhoemmen opened this issue Nov 6, 2023 · 0 comments
Open
19 of 22 tasks

P1673: LWG review 2023/11/06 (Kona) #425

mhoemmen opened this issue Nov 6, 2023 · 0 comments

Comments

@mhoemmen
Copy link
Contributor

mhoemmen commented Nov 6, 2023

P1673: LWG review 2023/11/06 (Kona) and follow-ons

Resuming from [linalg.alg.blas2.trsv], where we stopped last time (2023/10/25).

Done, either in the main branch or in PR #426

  • Generally, we need to qualify use of forward as std::forward. Change throughout.
  • Replace multiplyable with multipliable throughout. (Fix spelling.)
  • Fix parallel algorithms "element access function" wording in [algorithms.parallel.defns] 3, by adding "or mdspan types" after "iterators": "All operations of the categories of the iterators or mdspan types that the algorithm is instantiated with."
  • Add BinaryDivideOp to the list in [algorithms.parallel.user] (Requirements on user-provided function objects).
  • Generally, say "y may alias x" instead of "x and y may alias." This is consistent with the definition of "alias" in the front matter. Generally, "out-argument may alias in-argument." (PR P1673: LWG review (follow-on to PR 424) #426)
  • Change label xmm to xxmm. (DONE)
  • When passing an ExecutionPolicy exec into Effects-equivalent-to code, always use std::forward<ExecutionPolicy>(exec).
  • Fix matrix_rank_1_update* to use inout matrix as intended. Wording is wrong!
  • Change the updating wording to use $C'$ instead of relying on [linalg.general] 1.4. "Computes a matrix $C'$ such that $C' = C + \alpha A A^T$ and assigns each element of $C'$ to the corresponding element of $C$. Do this generally, e.g., for triangular_matrix_vector_product.
  • Change linalg.alg.blas3.* stable names to linalg.algs.blas3.*.
  • Fix stable names for BLAS 2 rank-1 symmetric and Hermitian updates.
  • For symmetric_matrix_rank_k_update and hermitian_matrix_rank_k_update, change the template parameter name InMat1 to InMat, because there's no InMat2. Fix this in the synopsis as well.
  • Fix everything under transposed (e.g., layout_transposed) to match wording of existing layouts. Remove references to the padded layouts.
  • Fix layout_blas_packed analogously.
  • Remove editorial comments and italics from [linalg.reqs.val].
  • For [linalg.algs.reqs] Para 1, avoid ambiguity by changing "type requirements" to "Constraints." However, the first (1.1) is more than just a Constraint, so separate (1.1) to the top, and add the Constraint that ExecutionPolicy is an execution policy. (This indicates that the algorithm is a parallel algorithm.)
  • Verify that the wording up front explains that ill-formed constructions (e.g., x and y complex, A real for hermitian rank-2 update) are forbidden (you may find an overload, but instantiating it would fail -- it's a precondition). (linalg.reqs.alg] expresses this for plus, times, etc. How about for complex number operations?) (See LWG question below.)

Check with LWG

  • For [linalg.algs.reqs] Para 1 (changing "type requirements" to "Constraints"; see above): Fixed by changing "type requirements" to "Constraints." Added the Constraint that ExecutionPolicy is an execution policy. Removed the requirement that the algorithms that take ExecutionPolicy are parallel algorithms, because that would be circular with [algorithms.parallel] 2 ("A parallel algorithm is a function template listed in this document with a template parameter named ExecutionPolicy").
  • For [mdspan.layout.policy.reqmts], it's not clear whether this says that MP::mapping<E> must be valid for ANY specialization E of extents. That was not our intent. Should we add a Note or just add clarifying normative language? (The latter would probably not break existing user code, unless users are somehow depending on generic code being able to take any layout MP and any extents specialization E, and get MP::mapping<E> from it.) This matters for P1673 because layout_transposed::mapping<E> requires that E have rank 2.
    • Christian asked some LWG people; they said we might want to add a Note, but the wording itself permits this.
  • Remove constraint that stride(r) requires rank 2? Note that stride in layout_left and layout_right impose a Constraint that rank() > 0, but layout_stride only has this as a precondition.
  • Rename layout_transpose to layout_transposed?

Resolved (don't need to check with LWG)

  • Verify that the wording up front explains that ill-formed constructions (e.g., x and y complex, A real for hermitian rank-2 update) are forbidden (you may find an overload, but instantiating it would fail -- it's a precondition). (linalg.reqs.alg] expresses this for plus, times, etc. How about for complex number operations?)
    • We think [linalg.helpers.conj] partly covers this.
      • conj-if-needed(E) only evaluates to conj(E) if conj(E) is a valid expression and the type of E is not an arithmetic type. Otherwise, it evaluates to E.
      • If conj(E) is a valid expression but "the function selected by overload resolution does not return the complex conjugate of its input, the program is ill-formed, no diagnostic required."
    • The main question is whether conj(E) is a linear algebra value type.
    • Ditto for the few algorithms that use real and imag.
    • We don't need to ask this question.

Points for specific sections

[linalg.alg.blas2.trsv]

("take an x parameter" cannot be misread as "take a symbolic name of a parameter x"; "take a parameter x" can be misread in that way. Therefore, we prefer the existing wording, "take an x parameter.") (OK)

Para 6: "Computes a vector $x'$ such that..." (instead of just saying "Computes $x'$ ..."). (This permits nonunique $x'$.) (DONE)

Para 12: "Computes a vector $x'$ such that" (instead of just saying "Computes $x'$ ..."), as above. (DONE)

Para 13: Complexity method differs from in Para 7. Flip order of b and A in Para 13 to be consistent with Para 7. (DONE)

[linalg.alg.blas2.rank1]

Para 7: Put forward<ExecutionPolicy> around exec. Generally do this where needed. (DONE)

Split Para 7 into two sentences, to avoid possible confusion due to code-font semicolons. (DONE)

[linalg.alg.blas2.symrank1]

Para 3: Mandates here does not follow the decltype convention; change that. (DONE)

(LWG appreciates the Para 1 unambiguous self-reference : - ) . ) (OK)

Para 7: "Computes a matrix $A'$..." instead of "Computes $A'$...." (DONE)

Para 11: "Computes a matrix $A'$..." instead of "Computes $A'$...." (DONE)

[linalg.algs.blas2.rank2]

Verify that the wording up front explains that ill-formed constructions (e.g., x and y complex, A real for hermitian rank-2 update) are forbidden (you may find an overload, but instantiating it would fail -- it's a precondition). (linalg.reqs.alg] expresses this for plus, times, etc. How about for complex number operations?) (DONE -- see question for LWG)

[linalg.algs.blas3.gemm]

Mandates: the above and also? (see notes)

Scribe missed some time

The in-place triangular_matrix_right_product etc. need a complexity clause. (DONE)

[linalg.alg.blas3.rankk]

Change label from linalg.alg to linalg.algs here. (DONE)

For symmetric_matrix_rank_k_update function signature, change InMat1 to InMat, because there's no InMat2. Fix this in the synopsis as well. (DONE)

Para 6: Change the updating wording to use $C'$ instead of relying on [linalg.general] 1.4. "Computes a matrix $C'$ such that $C' = C + \alpha A A^T$ and assigns each element of $C'$ to the corresponding element of $C$. Generally do that. It's OK to leave [linalg.general] 1.4, as it is still needed. (We'll need to fix in-place triangular_matrix_vector_product ("Computes $y = A y$"). (DONE)

For [linalg.algs.reqs] Para 1, does "type requirements" mean constraints? Yes, it does, to avoid ambiguity. Therefore, change "type requirements" to "Constraints." However, the first (1.1) is more than just a Constraint. We can separate (1.1) to the top, add the Constraint that ExecutionPolicy is an execution policy. (This indicates that the algorithm is a parallel algorithm.) (DONE)

[linalg.alg.blas3.rank2k]

Change linalg.alg to linalg.algs. (DONE)

Fix Effects generally. (DONE)

For next time

Next starting point is BLAS 3 TRSM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant