chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.75k stars 410 forks source link

Change read-only scalar arguments from 'ref' to default intent in BLAS routines #24898

Closed bradcray closed 3 weeks ago

bradcray commented 3 weeks ago

Several of the alpha/beta arguments in our BLAS routines have historically been declared ref because the Fortran implementations of the routines (and their C headers) pass the scalar values in question using a pointer when using complex values. However, these should have been const ref arguments in order to permit const and param expressions to be passed in for these arguments, which are logically "in" arguments and read-only.

Here, I'm taking that change one step further and just making the user-facing Chapel routines take these scalar arguments by default intent, then applying a c_ptrToConst() within the body of the routine to pass the value along to the underlying C routines by pointer-to-const as they want. This has the benefits of supporting a simpler interface / avoiding questions as to why these would be passed by ref in the first place, and potentially supporting multi-locale runs better by virtue of localizing remote scalar values before passing them to C.

In support of this, I've changed the extern declarations from using ref on scalar arguments to using c_ptr() types that match the cblas.h header file provided by OpenBLAS. Though this work was originally motivated by the alpha/beta arguments—and those make up the lion's share of ref scalar arguments, I reviewed the extern procs for other ref scalar arguments and updated them to use pointers as well, for consistency. This reflects the style that I think we prefer for extern C routines these days, which is to describe the interface C expects rather than to Chapel-ify it using ref.

Updated tests that call the user-facing routines affected by these changes to have them pass in const to lock in the change (when such a test existed—some of the routines I updated do not seem to currently have tests).

TODO:

bradcray commented 3 weeks ago

Here's an example of what the impact is on the user-facing docs using one of the simpler BLAS routines. Generally, these routines are complex enough that it's not a huge change, but still nice I think:

old:

Screen Shot 2024-04-23 at 11 52 03 AM

new:

Screen Shot 2024-04-22 at 3 38 44 PM
ninivert commented 3 weeks ago

Here's an example of what the impact is on the user-facing docs using one of the simpler BLAS routines. Generally, these routines are complex enough that it's not a huge change, but still nice I think:

old: Screen Shot 2024-04-22 at 3 38 34 PM

new: Screen Shot 2024-04-22 at 3 38 44 PM

I noticed also when merging commit da6e4315479 (@riftEmber) into my working branch, that the inout intents previously were erroneously not marked as ref for the corresponding arguments (e.g. here X -> ref X).

The commit messages say that the checks were done manually, but seeing the sheer amount of functions, maybe it would be a good idea to automate this ?

riftEmber commented 3 weeks ago

@ninivert Yeah, we only noticed and were prompted to fix the missing ref intents when https://github.com/chapel-lang/chapel/pull/24746 removed the warning suppression for dropped const qualifiers in generated C (which are implicit from default array intent). It was not fun and probably not 100% accurate to manually reference the docs and update the functions; a script to do so would be good to have.

We have something similar for the LAPACK package module in util/misc/gen-LAPACK, but it hasn't been kept up to date with manual changes made to the module for reasons I'm not aware of. I think updating that existing script is higher in the queue, but after that it could probably be used as a model for a script to generate the BLAS routines -- I'll make a note of it. It's definitely on our radar though not scheduled yet.

bradcray commented 3 weeks ago

I think the external C_BLAS routines may have originally been generated by a script as well, but if so — as often happens with these things — has then been manually updated directly rather than through the script (whetherdue to laziness, not knowing that the script exists or where it is, or the script not working as well as BLAS itself has evolved).

It'd be interesting to run cblas.h through c2chapel and see how close or far the output was from the C_BLAS sub-module today.

bradcray commented 3 weeks ago

(also, my inclusion of the change in ref to the array argument in the visual diff was unintentional since it was unrelated to this PR; I've updated the image to use the docs from main with the ref rather than the 2.0 docs that I'd used yesterday).