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

Refactor OperatorMsg #3764

Closed

Conversation

jeremiah-corrado
Copy link
Contributor

@jeremiah-corrado jeremiah-corrado commented Sep 11, 2024

Refactors OperatorMsg to use instantiateAndRegister with the goal of making the code clearer and more readable. Closes #3719

Previously, this module had a few monolithic message handlers that selected over all pairs of dytpes. Within each when-clause, the implementation would figure out the result type based on the kind of operator, and then call out to helper procedures in binOps.chpl that had various conditionals over the array types and/or the result array type to select the correct implementation of the operator (or return a not supported message) based on the types involved.

I found this scheme somewhat difficult to follow when reading/modifying the operatorMsg code. As such, the refactored implementation, has one message-handler for each kind of operator — where the goal is to have a consistently defined result type within that procedure. In some cases the procedures are overloaded for array element types like bool or bigint that require special implementations.

I'm hoping that the simplified layout leads to faster compilation and/or reduced memory consumption when compiling the module. I have yet to test this.

jeremiah-corrado and others added 4 commits September 10, 2024 08:41
…arate procedures for different kinds of binary operations (to avoid unused code paths)

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
@jeremiah-corrado jeremiah-corrado changed the title Refactor binops Refactor OperatorMsg Sep 12, 2024
jeremiah-corrado and others added 7 commits September 12, 2024 11:31
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
@jeremiah-corrado
Copy link
Contributor Author

Note: tests/numpy/numeric_test.py::test_putmask appears to be hanging in CI. It's not obvious why, and I've been unable to reproduce this locally so far (even with CHPL_RT_NUM_THREADS_PER_LOCALE=2). I'm still investigating.

@jeremiah-corrado jeremiah-corrado marked this pull request as ready for review September 13, 2024 18:02
@jeremiah-corrado
Copy link
Contributor Author

closing for now in favor of a simpler refactor: #3773

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

Successfully merging this pull request may close these issues.

Remove @arkouda.registerND from OperatorMsg.chpl.
1 participant