Add flagcx event type for device event creation #275

mikethegoblin · 2025-10-22T07:40:37Z

added enum type flagcxEventType_t to distinguish the types of event to be created

gemini-code-assist · 2025-10-22T07:41:05Z

Summary of Changes

Hello @mikethegoblin, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the flagcx framework by introducing a new flagcxEventType_t enumeration. This allows for explicit control over the type of events created, specifically enabling the distinction between default events and those configured to disable timing. This change propagates through the device adaptors, core logic, and the Python wrapper, providing greater flexibility and optimization opportunities for event management within the system.

Highlights

New Event Type Enum: Introduced flagcxEventType_t to distinguish between default events and those configured to disable timing.
eventCreate Signature Update: The eventCreate function across all device adaptors and their declarations (adaptor.h, flagcx.h) now accepts the new flagcxEventType_t parameter.
Conditional Event Creation: Device-specific adaptors (CUDA, HIP, MLU, etc.) now use the eventType parameter to create events with either default flags or flags that disable timing, enhancing flexibility.
Existing Call Site Updates: All existing calls to eventCreate within the flagcx core (e.g., group.cc, launch_kernel.h, transport.cc, timer.h) have been updated to explicitly pass the appropriate flagcxEventType_t.
Python Wrapper Alignment: The Python FFI wrapper (flagcx_wrapper.py) has been updated to reflect the new eventCreate function signature, ensuring compatibility.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a new enum flagcxEventType_t to control event creation properties, specifically for enabling or disabling timing. The changes are propagated through the adaptor layer and to the call sites.

I've found a few issues:

A critical bug in hip_adaptor.cc where an assignment is used instead of a comparison.
A compilation error in mlu_adaptor.cc due to a typo.
The new eventType parameter is ignored in cann_adaptor.cc.

Additionally, I've suggested a minor refactoring for conciseness in several of the CUDA-like adaptors. Please review the comments for details.

flagcx/adaptor/device/hip_adaptor.cc

flagcx/adaptor/device/mlu_adaptor.cc

flagcx/adaptor/device/cann_adaptor.cc

flagcx/adaptor/device/cuda_adaptor.cc

mikethegoblin · 2025-10-23T02:48:13Z

@gemini-code-assist please review this pull request

gemini-code-assist

Code Review

This pull request introduces a new enum flagcxEventType_t to specify whether an event should be created with timing capabilities enabled or disabled. This is a valuable optimization, as events used purely for synchronization can be created more efficiently. The changes are consistently applied across all device adaptors and their respective call sites in the C++ code. The logic for selecting the appropriate event creation flags is correct in all instances. I've found one critical issue in the Python wrapper where a new type is used without being defined, which would lead to a runtime error.

plugin/interservice/flagcx_wrapper.py

mikethegoblin · 2025-10-24T07:02:08Z

@gemini-code-assist please review this pull request in detail

gemini-code-assist

Code Review

This pull request introduces a flagcxEventType_t enum to differentiate between event creation types, primarily for enabling or disabling timing information. The change is consistently applied across all device adaptors and their call sites. My review includes two main points: a suggestion to improve code safety and clarity in all adaptor implementations of eventCreate by avoiding a potentially brittle C-style cast, and a recommendation to add comments to the new enum definition to make its purpose clearer for future maintenance.

gemini-code-assist · 2025-10-24T07:04:05Z

flagcx/adaptor/device/cann_adaptor.cc

-  DEVCHECK(aclrtCreateEventWithFlag((aclrtEvent *)(*event), ACL_EVENT_SYNC));
+  const unsigned int flags =
+      (eventType == flagcxEventDefault) ? ACL_EVENT_TIME_LINE : ACL_EVENT_SYNC;
+  DEVCHECK(aclrtCreateEventWithFlag((aclrtEvent *)(*event), flags));


The cast (aclrtEvent *)(*event) is unsafe as it relies on the memory layout of struct flagcxEvent. A safer and more explicit approach is to take the address of the base member directly. This avoids potential issues if the struct definition changes in the future.

DEVCHECK(aclrtCreateEventWithFlag(&((*event)->base), flags));

gemini-code-assist · 2025-10-24T07:04:05Z

flagcx/adaptor/device/cuda_adaptor.cc

+  const unsigned int flags = (eventType == flagcxEventDefault)
+                                 ? cudaEventDefault
+                                 : cudaEventDisableTiming;
+  DEVCHECK(cudaEventCreateWithFlags((cudaEvent_t *)(*event), flags));


The cast (cudaEvent_t *)(*event) is unsafe as it relies on the memory layout of struct flagcxEvent. A safer and more explicit approach is to take the address of the base member directly. This avoids potential issues if the struct definition changes in the future.

DEVCHECK(cudaEventCreateWithFlags(&((*event)->base), flags));

gemini-code-assist · 2025-10-24T07:04:05Z

flagcx/adaptor/device/ducuda_adaptor.cc

+  const unsigned int flags = (eventType == flagcxEventDefault)
+                                 ? cudaEventDefault
+                                 : cudaEventDisableTiming;
+  DEVCHECK(cudaEventCreateWithFlags((cudaEvent_t *)(*event), flags));


The cast (cudaEvent_t *)(*event) is unsafe as it relies on the memory layout of struct flagcxEvent. A safer and more explicit approach is to take the address of the base member directly. This avoids potential issues if the struct definition changes in the future.

Suggested change

DEVCHECK(cudaEventCreateWithFlags((cudaEvent_t *)(*event), flags));

DEVCHECK(cudaEventCreateWithFlags(&((*event)->base), flags));

gemini-code-assist · 2025-10-24T07:04:06Z

flagcx/adaptor/device/hip_adaptor.cc

+  const unsigned int flags = (eventType == flagcxEventDefault)
+                                 ? hipEventDefault
+                                 : hipEventDisableTiming;
+  DEVCHECK(hipEventCreateWithFlags((hipEvent_t *)(*event), flags));


The cast (hipEvent_t *)(*event) is unsafe as it relies on the memory layout of struct flagcxEvent. A safer and more explicit approach is to take the address of the base member directly. This avoids potential issues if the struct definition changes in the future.

Suggested change

DEVCHECK(hipEventCreateWithFlags((hipEvent_t *)(*event), flags));

DEVCHECK(hipEventCreateWithFlags(&((*event)->base), flags));

gemini-code-assist · 2025-10-24T07:04:06Z

flagcx/adaptor/device/ixcuda_adaptor.cc

+  const unsigned int flags = (eventType == flagcxEventDefault)
+                                 ? cudaEventDefault
+                                 : cudaEventDisableTiming;
+  DEVCHECK(cudaEventCreateWithFlags((cudaEvent_t *)(*event), flags));


The cast (cudaEvent_t *)(*event) is unsafe as it relies on the memory layout of struct flagcxEvent. A safer and more explicit approach is to take the address of the base member directly. This avoids potential issues if the struct definition changes in the future.

Suggested change

DEVCHECK(cudaEventCreateWithFlags((cudaEvent_t *)(*event), flags));

DEVCHECK(cudaEventCreateWithFlags(&((*event)->base), flags));

gemini-code-assist · 2025-10-24T07:04:06Z

flagcx/adaptor/device/kunlunxin_adaptor.cc

+  const unsigned int flags = (eventType == flagcxEventDefault)
+                                 ? cudaEventDefault
+                                 : cudaEventDisableTiming;
+  DEVCHECK(cudaEventCreateWithFlags((cudaEvent_t *)(*event), flags));


The cast (cudaEvent_t *)(*event) is unsafe as it relies on the memory layout of struct flagcxEvent. A safer and more explicit approach is to take the address of the base member directly. This avoids potential issues if the struct definition changes in the future.

Suggested change

DEVCHECK(cudaEventCreateWithFlags((cudaEvent_t *)(*event), flags));

DEVCHECK(cudaEventCreateWithFlags(&((*event)->base), flags));

gemini-code-assist · 2025-10-24T07:04:06Z

flagcx/adaptor/device/maca_adaptor.cc

-  DEVCHECK(mcEventCreateWithFlags((mcEvent_t *)(*event), mcEventDisableTiming));
+  const unsigned int flags =
+      (eventType == flagcxEventDefault) ? mcEventDefault : mcEventDisableTiming;
+  DEVCHECK(mcEventCreateWithFlags((mcEvent_t *)(*event), flags));


The cast (mcEvent_t *)(*event) is unsafe as it relies on the memory layout of struct flagcxEvent. A safer and more explicit approach is to take the address of the base member directly. This avoids potential issues if the struct definition changes in the future.

Suggested change

DEVCHECK(mcEventCreateWithFlags((mcEvent_t *)(*event), flags));

DEVCHECK(mcEventCreateWithFlags(&((*event)->base), flags));

gemini-code-assist · 2025-10-24T07:04:06Z

flagcx/adaptor/device/mlu_adaptor.cc

+  const unsigned int flags = (eventType == flagcxEventDefault)
+                                 ? CNRT_NOTIFIER_DEFAULT
+                                 : CNRT_NOTIFIER_DISABLE_TIMING_ALL;
+  DEVCHECK(cnrtNotifierCreateWithFlags((cnrtNotifier_t *)(*event), flags));


The cast (cnrtNotifier_t *)(*event) is unsafe as it relies on the memory layout of struct flagcxEvent. A safer and more explicit approach is to take the address of the base member directly. This avoids potential issues if the struct definition changes in the future.

DEVCHECK(cnrtNotifierCreateWithFlags(&((*event)->base), flags));

gemini-code-assist · 2025-10-24T07:04:06Z

flagcx/adaptor/device/musa_adaptor.cc

+  const unsigned int flags = (eventType == flagcxEventDefault)
+                                 ? musaEventDefault
+                                 : musaEventDisableTiming;
+  DEVCHECK(musaEventCreateWithFlags((musaEvent_t *)(*event), flags));


The cast (musaEvent_t *)(*event) is unsafe as it relies on the memory layout of struct flagcxEvent. A safer and more explicit approach is to take the address of the base member directly. This avoids potential issues if the struct definition changes in the future.

Suggested change

DEVCHECK(musaEventCreateWithFlags((musaEvent_t *)(*event), flags));

DEVCHECK(musaEventCreateWithFlags(&((*event)->base), flags));

flagcx/include/flagcx.h

MC952-arch · 2025-10-24T09:37:05Z

@gemini-code-assist please review this pull request in details

gemini-code-assist

Code Review

This pull request introduces a flagcxEventType_t enum to allow for more granular control over device event creation, specifically to enable or disable timing. The changes are consistently applied across all device adaptors, mapping the generic event type to the correct device-specific flags. Call sites have been updated to use the new functionality, enabling timing for profiler events in timer.h and disabling it for internal synchronization events in group.cc, launch_kernel.h, and transport.cc for better performance. The refactoring of event handle access from a C-style cast to accessing a base member of the event struct is also a good improvement for type safety and code clarity. Overall, this is a well-executed and beneficial change.

MC952-arch

LGTM

gemini-code-assist bot reviewed Oct 22, 2025

View reviewed changes

flagcx/adaptor/device/hip_adaptor.cc Outdated Show resolved Hide resolved

flagcx/adaptor/device/mlu_adaptor.cc Outdated Show resolved Hide resolved

flagcx/adaptor/device/cann_adaptor.cc Outdated Show resolved Hide resolved

flagcx/adaptor/device/cuda_adaptor.cc Outdated Show resolved Hide resolved

gemini-code-assist bot reviewed Oct 23, 2025

View reviewed changes

plugin/interservice/flagcx_wrapper.py Show resolved Hide resolved

mikethegoblin added 5 commits October 24, 2025 15:00

add flagcx event type as input to adaptor API

0c3d90f

fix missing character in hip adaptor

32c4797

update cann event type

902174e

simplify conditional statement for flags

107b651

define flagcxEventType_t in flagcx_wrapper.py

f20b472

mikethegoblin force-pushed the main branch from 6dbb6fe to f20b472 Compare October 24, 2025 07:01

gemini-code-assist bot reviewed Oct 24, 2025

View reviewed changes

fix unsafe cast

84b6ecc

gemini-code-assist bot reviewed Oct 24, 2025

View reviewed changes

MC952-arch approved these changes Oct 24, 2025

View reviewed changes

	DEVCHECK(cudaEventCreateWithFlags((cudaEvent_t )(event), flags));
	DEVCHECK(cudaEventCreateWithFlags(&((*event)->base), flags));

	DEVCHECK(hipEventCreateWithFlags((hipEvent_t )(event), flags));
	DEVCHECK(hipEventCreateWithFlags(&((*event)->base), flags));

	DEVCHECK(mcEventCreateWithFlags((mcEvent_t )(event), flags));
	DEVCHECK(mcEventCreateWithFlags(&((*event)->base), flags));

	DEVCHECK(musaEventCreateWithFlags((musaEvent_t )(event), flags));
	DEVCHECK(musaEventCreateWithFlags(&((*event)->base), flags));

Add flagcx event type for device event creation #275

Are you sure you want to change the base?

Add flagcx event type for device event creation #275

Uh oh!

Conversation

mikethegoblin commented Oct 22, 2025

Uh oh!

gemini-code-assist bot commented Oct 22, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikethegoblin commented Oct 23, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

mikethegoblin commented Oct 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MC952-arch commented Oct 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

MC952-arch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants