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

bli_scalv has inconsistent semantics #829

Open
devinamatthews opened this issue Sep 23, 2024 · 1 comment
Open

bli_scalv has inconsistent semantics #829

devinamatthews opened this issue Sep 23, 2024 · 1 comment

Comments

@devinamatthews
Copy link
Member

The operations bli_(inv)?scal[vdm] are alone in having a conjalpha parameter (in the typed API and kernel definitions), while also taking an input vector (matrix diagonal, matrix) x which does not carry a conjugation flag. Functionally, this means that there is no operation which has the semantics of in-place conjugation. This is also in contrast to all other level1 operations which support arbitrary conjugation of inputs, while conversely not supporting conjugation flags for scalars. The value of conjalpha for its present purpose is debatable since it is trivial to conjugate scalars externally, and as just mentioned is not supported elsewhere (via the typed API at least). In principle, the functions bli_scal2[vdm] can be used to achieve conjugation of x, although there are currently bugs in the implementation for complex values (#828). No alternative exists for bli_invscal[vdm].

Note that bli_set[vdm] also has conjalpha which is harmless, although in the same vein arguably superfluous.

Ideally I would propose changing the semantics of the conjalpha to conjx to better align with other level1 functions and enable easy (and correct) in-place conjugation with optional scaling. However, I wonder: is anyone currently using bli_scal[vdm] with the conjalpha parameter for its present purpose? This would be a breaking API change... For the narrow use case of conjugation only, a separate bli_conj[vdm] function would work, but for example in TBLIS I often need both scaling and conjugation at the same time.

@fgvanzee
Copy link
Member

I agree that conj[vdm] would be useful. While I considered them, they didn't made the cut when I was originally formulating the operations to include in BLIS.

As for the conjalpha parameter in scalv, I agree that it's arguable whether input-only scalars should ever have conj parameters. I would be fine with settling on "no" to that question.

On conjx: I'm inclined to think that adding a conjx parameter to scal[vdm] would make the resulting operations different enough that they should just be new operations. (I'm kind of of the opinion that in-place conjugation shouldn't be combined with anything else, since it would introduce its own kind of inconsistency with the other operations supported within BLIS.)

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

2 participants