Skip to content

[libspirv][NFC] Fix build warnings when -Wall -Wextra is enabled#21586

Open
wenju-he wants to merge 1 commit intointel:syclfrom
wenju-he:fix-libspirv-build-warnings
Open

[libspirv][NFC] Fix build warnings when -Wall -Wextra is enabled#21586
wenju-he wants to merge 1 commit intointel:syclfrom
wenju-he:fix-libspirv-build-warnings

Conversation

@wenju-he
Copy link
Contributor

No description provided.

@wenju-he wenju-he requested review from a team and Maetveis as code owners March 23, 2026 00:00
@wenju-he wenju-he requested a review from kweronsx March 23, 2026 00:00
Comment on lines 10 to +12
int scope, global __CLC_GENTYPE *dst, const local __CLC_GENTYPE *src,
size_t num_gentypes, size_t stride, event_t event) {
(void)scope;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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 :)

Comment on lines +327 to +329
} else { \
__builtin_trap(); \
__builtin_unreachable(); \
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

} else { \
result = OP(sg_x, sg_prefix); \
} \
} else { \
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here.

Comment on lines -2579 to +2585
float4 a = __nvvm_tex_1d_v4f32_f32(imageHandle, x);
float4 a = __nvvm_tex_3d_v4f32_f32(imageHandle, x, y, z);
Copy link
Contributor

Choose a reason for hiding this comment

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

How did this ever work before? Good catch, ...I think :).

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.

2 participants