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

Fix calls in caffeine.c #108

Merged
merged 4 commits into from
Apr 10, 2024
Merged

Fix calls in caffeine.c #108

merged 4 commits into from
Apr 10, 2024

Conversation

ktras
Copy link
Collaborator

@ktras ktras commented Apr 9, 2024

Closes #74.

@ktras
Copy link
Collaborator Author

ktras commented Apr 9, 2024

The following two warnings still exist because when attempting to remove the warnings by dereferencing the user_op args to match what the functions seems to expect, then there is a runtime crash during the tests. These function calls involves macro expansions for both the functions and for the argument types, so I may have misread what was needed to fix the call and remove the warning. Or perhaps the call doesn't need fixing.

././src/caffeine/caffeine.c:142:103: warning: passing argument 9 of 'gasnete_tm_reduce_nb' from incompatible pointer type [-Wincompatible-pointer-types]
  142 |       team, result_image-1, a_address, a_address, GEX_DT_USER, c_sizeof_a, num_elements, GEX_OP_USER, user_op, &c_sizeof_a, 0
      |                                                                                                       ^~~~~~~
      |                                                                                                       |
      |                                                                                                       void (**)(const void *, void *, size_t,  const void *) {aka void (**)(const void *, void *, long unsigned int,  const void *)}
/Users/kateras/.local/include/gasnet_coll.h:400:60: note: in definition of macro 'gex_Coll_ReduceToOneNB'
  400 |         gasnete_tm_reduce_nb(tm,root,dst,src,dt,dts,dtc,op,fn,cdata,flags,0 GASNETI_THREAD_GET)
      |                                                            ^~
/Users/kateras/.local/include/gasnet_coll.h:396:58: note: expected 'gex_Coll_ReduceFn_t' {aka 'void (*)(const void *, void *, long unsigned int,  const void *)'} but argument is of type 'void (**)(const void *, void *, size_t,  const void *)' {aka 'void (**)(const void *, void *, long unsigned int,  const void *)'}
  396 |                        gex_OP_t _op, gex_Coll_ReduceFn_t _user_op, void * _user_cdata,
      |                                      ~~~~~~~~~~~~~~~~~~~~^~~~~~~~
././src/caffeine/caffeine.c:146:103: warning: passing argument 8 of 'gasnete_tm_reduce_all_nb' from incompatible pointer type [-Wincompatible-pointer-types]
  146 |       team,                 a_address, a_address, GEX_DT_USER, c_sizeof_a, num_elements, GEX_OP_USER, user_op, &c_sizeof_a, 0
      |                                                                                                       ^~~~~~~
      |                                                                                                       |
      |                                                                                                       void (**)(const void *, void *, size_t,  const void *) {aka void (**)(const void *, void *, long unsigned int,  const void *)}
/Users/kateras/.local/include/gasnet_coll.h:413:59: note: in definition of macro 'gex_Coll_ReduceToAllNB'
  413 |         gasnete_tm_reduce_all_nb(tm,dst,src,dt,dts,dtc,op,fn,cdata,flags,0 GASNETI_THREAD_GET)
      |                                                           ^~
/Users/kateras/.local/include/gasnet_coll.h:409:59: note: expected 'gex_Coll_ReduceFn_t' {aka 'void (*)(const void *, void *, long unsigned int,  const void *)'} but argument is of type 'void (**)(const void *, void *, size_t,  const void *)' {aka 'void (**)(const void *, void *, long unsigned int,  const void *)'}
  409 |                         gex_OP_t _op, gex_Coll_ReduceFn_t _user_op, void * _user_cdata,
      |                                       ~~~~~~~~~~~~~~~~~~~~^~~~~~~~

@bonachea
Copy link
Member

bonachea commented Apr 9, 2024

The following two warnings still exist because when attempting to remove the warnings by dereferencing the user_op args to match what the functions seems to expect, then there is a runtime crash during the tests.

I suspect you deployed the wrong fix. This is a warning about mismatched types in code that is already believed to operate correctly. In such a situation adding or removing a deference operator to assuage the typechecker is almost never the right fix, because it changes the semantics of the code that already runs correctly (to something that usually does not).

I believe the actual problem here is with the type declarations, specifically the C declaration of the caf_co_reduce function. gex_Coll_ReduceFn_t is already a pointer-to-function type, and thus compatible with the c_funptr provided from the Fortran side of the call. The problem is the extraneous * in the C declaration of the user_op formal argument to caf_co_reduce.

Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes so far look right.

I agree there are several problems revealed in issue #74 that remain unaddressed so far. I'd say it's your choice whether to merge this incremental improvement as-is, or attempt to fix the rest of the warnings here.

On a related note, I'd like to suggest/request a convention where commits which fix or address issues include the issue number in the commit message. Right now the issue number appears only in the PR description, but years from now it will be much easier to understand the commit in isolation if the issue number also appears in the commit message.

Copy link
Member

@everythingfunctional everythingfunctional left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, but I'll defer to Dan on the intricacies of C type-casting.

@ktras ktras requested a review from bonachea April 10, 2024 16:22
@ktras ktras merged commit c717ab2 into main Apr 10, 2024
6 checks passed
@ktras ktras deleted the fix-c-calls branch April 10, 2024 17:16
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 warnings from gcc by fixing calls in caffeine.c
3 participants