Skip to content

fix the kernel details for v1#662

Open
cj401-amd wants to merge 2 commits intorocm-jaxlib-v0.6.0from
fix_v1_kernel_details_rocm-jaxlib-v0.6.0
Open

fix the kernel details for v1#662
cj401-amd wants to merge 2 commits intorocm-jaxlib-v0.6.0from
fix_v1_kernel_details_rocm-jaxlib-v0.6.0

Conversation

@cj401-amd
Copy link

Motivation

fix the kernel details for v1, in case we still need to switch to v1 due to issues with rocprofiler-sdk. (later versions have such issues).

@cj401-amd cj401-amd added the bug Something isn't working label Mar 9, 2026
@cj401-amd cj401-amd requested a review from gabeweisz March 9, 2026 20:49
Copy link
Author

@cj401-amd cj401-amd left a comment

Choose a reason for hiding this comment

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

addressed.


#define XLA_FFI_UNKNOWN_TYPE_ID XLA_FFI_TypeId{0}
#define XLA_FFI_UNKNOWN_TYPE_ID \
XLA_FFI_TypeId { 0 }
Copy link

Choose a reason for hiding this comment

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

Potential C compatibility issue: This macro was reformatted from XLA_FFI_TypeId{0} to XLA_FFI_TypeId { 0 } (space between type name and opening brace). This header is a C/C++ interop header (guarded by #ifdef __cplusplus / extern "C"). While XLA_FFI_TypeId { 0 } is valid brace-initialization in C++, if this macro is ever expanded in C code it could cause issues — compound literals in C require (XLA_FFI_TypeId){0} syntax. The original form was safer. Consider reverting this formatting change.

EXPECT_TRUE(state.IsSet());

TF_ASSERT_OK_AND_ASSIGN(int32_t* data, state.Get<int32_t>());
TF_ASSERT_OK_AND_ASSIGN(int32_t * data, state.Get<int32_t>());
Copy link

Choose a reason for hiding this comment

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

Style nit: int32_t * data (space on both sides of *) does not match Google C++ style, which uses int32_t* data. The formatter may have been confused by the TF_ASSERT_OK_AND_ASSIGN macro's comma. Consider reverting to int32_t* data.


// Record elapsed device time for the given input device type.
void Record(absl::Duration elapsed, DeviceType device_type) override {};
void Record(absl::Duration elapsed, DeviceType device_type) override{};
Copy link

Choose a reason for hiding this comment

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

Formatting inconsistency: The space before override was removed here (override{};), but the line above keeps the space (override {). This looks like an accidental formatting regression — consider adding the space back for consistency and readability: override {};.

@cj401-amd cj401-amd force-pushed the fix_v1_kernel_details_rocm-jaxlib-v0.6.0 branch from d7c9709 to 1d7e952 Compare March 10, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants