[libspirv][NFC] Fix build warnings when -Wall -Wextra is enabled#21586
[libspirv][NFC] Fix build warnings when -Wall -Wextra is enabled#21586wenju-he wants to merge 3 commits intointel:syclfrom
Conversation
| int scope, global __CLC_GENTYPE *dst, const local __CLC_GENTYPE *src, | ||
| size_t num_gentypes, size_t stride, event_t event) { | ||
| (void)scope; |
There was a problem hiding this comment.
| int scope, global __CLC_GENTYPE *dst, const local __CLC_GENTYPE *src, | |
| size_t num_gentypes, size_t stride, event_t event) { | |
| (void)scope; | |
| int /*scope*/, global __CLC_GENTYPE *dst, const local __CLC_GENTYPE *src, | |
| size_t num_gentypes, size_t stride, event_t event) { |
NIT: I'm not sure if libclc has an established existing practice for unused parameters, if it does it might be better to follow that, but I personally prefer this style. See also C++ Core Guidelines - Unused parameters should unnamed
Applies to all the places where there are unused parameters of course, I don't want to comment on all of them :)
There was a problem hiding this comment.
int /*scope*/ is not exact the same as the guideline since it contains comment?
There was a problem hiding this comment.
Yes true, I think it is in the spirit of the guidelines to have a comment, and a name is a good short comment of what that parameter would be if it were used (maybe users should still pass it, because the function might start using it in the future).
I don't feel strongly about this though, so if you prefer to completely get rid of the name that's okay for me too.
There was a problem hiding this comment.
BTW I tried grepping for what is more common in the LLVM codebase. I can find examples of having the comment, but trying to search for the other case brings up too much noise (calls to functions instead of definitions, casts, etc) with a simple regex so I gave up.
| } else { \ | ||
| __builtin_trap(); \ | ||
| __builtin_unreachable(); \ |
There was a problem hiding this comment.
If op is ever not one of the above values that is an error made by the developer of libclc, and never the user, right?
In that case I think this check should be some kind of build-time controlled assertion that compiles to only __builtin_unreachable in release builds (basically LLVM_UNREACHABLE).
I would also find a structure like this better, in which case we don't need unreachable:
if (op == Reduce) { \
result = __clc__SubgroupShuffle(x, __spirv_BuiltInSubgroupSize() - 1); \
*carry = result; \
} /* For InclusiveScan, use results as computed */ \
else if (op == InclusiveScan) { \
result = x; \
*carry = result; \
} else { \
LIBCLC_ASSERT(op == ExclusiveScan && "Unexpected operation"); \
/* For ExclusiveScan, shift and prepend identity */ \
*carry = x; \
result = __clc__SubgroupShuffleUp(x, 1); \
if (sg_lid == 0) { \
result = IDENTITY; \
} \
}Also we're basically expecting the optimizer to fold these conditionals out, correct?
In this case I think just as OP is a macro, we could have a EPILOG_OP. Though honestly using some C++ templates and lambdas in libclc is sounding more and more attractive.
There was a problem hiding this comment.
If you'd like to get this in quickly and return to LIBCLC_ASSERT or similar in a follow up then I would make the code like the above and only have the "assert" as a TODO comment.
There was a problem hiding this comment.
thanks for the advice. I have change the code to set result to 0 when group operation is unsupported. This aligns with IGC builtin implementation behavior.
There was a problem hiding this comment.
This aligns with IGC builtin implementation behavior.
I don't think we need to necessarily align with that. Could you at the minimum add a TODO comment here and mention that these branches are never expected to be hit?
There was a problem hiding this comment.
Could you at the minimum add a TODO comment here
done
There was a problem hiding this comment.
/* TODO: Not implemented yet */
This is not the comment text I was expecting, and it now makes me question if I understand this code correctly or not. What is not implemented here, are there supposed to be more collective operations, but we are not supporting all of them?
In my first comment I asked this:
If op is ever not one of the above values that is an error made by the developer of libclc, and never the user, right?
You never answered this, I assumed because I was making the right assumption, but now I'm not so sure.
Can you please confirm for me that this code-path is never actually expected to be hit? Even if the user passes values to opencl library functions that are not allowed per the spec?
There was a problem hiding this comment.
this code-path is never actually expected to be hit
I think this code path can be hit, for example, IGC 2.0 supports ClusteredReduce for few group functions.
libspirv implementation SPIR-V function in spec, so all opcode in the spec can be expected.
If op is ever not one of the above values that is an error made by the developer of libclc, and never the user, right?
Even if the user passes values to opencl library functions that are not allowed per the spec?
It could be something gap between what have been implemented and what are allowed.
I think it is fine to either ignore invalid input or put a unreachable for all unimplemented paths.
| } else { \ | ||
| result = OP(sg_x, sg_prefix); \ | ||
| } \ | ||
| } else { \ |
There was a problem hiding this comment.
set result to 0 when group operation is unsupported
2 participants
No description provided.