Skip to content

Conversation

majing921201
Copy link
Contributor

@majing921201 majing921201 commented Sep 19, 2025

Switch philox_engine_inputs usage to philox_xpu_state per XPU graph request. This PR requires stock pytorch XPUGeneratorImpl PR merged.

@Copilot Copilot AI review requested due to automatic review settings September 19, 2025 10:04
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the XPU implementation to use the new philox_xpu_state API instead of the legacy philox_engine_inputs API to support XPU graph functionality. The changes replace the old pair-based approach for Philox random number generation with a unified state object.

  • Replace philox_engine_inputs() calls with philox_xpu_state()
  • Update function signatures to use PhiloxXpuState instead of std::pair<uint64_t, uint64_t>
  • Replace philox_unpack() with at::xpu::philox::unpack()

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/ATen/native/xpu/sycl/RreluWithNoiseKernels.cpp Updates RReLU with noise kernel to use new philox state API
src/ATen/native/xpu/sycl/RandpermKernel.cpp Updates randperm kernel to use new philox state API and removes unused include
src/ATen/native/xpu/sycl/MultinomialKernel.cpp Updates multinomial kernel to use new philox state API
src/ATen/native/xpu/sycl/Dropout.cpp Updates dropout kernels to use new philox state API
src/ATen/native/xpu/sycl/Distributions.cpp Updates various distribution kernels to use new philox state API
src/ATen/native/xpu/sycl/DistributionTemplates.h Removes PhiloxState definition and updates template functions to use new API

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@@ -1,5 +1,4 @@
#include <ATen/Dispatch.h>
#include <ATen/native/xpu/sycl/DistributionTemplates.h>
#include <ATen/native/xpu/sycl/Philox4x32.h>
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The removed include <ATen/native/xpu/sycl/DistributionTemplates.h> might still be needed since this file uses functions that were previously defined there. Verify that all necessary functions are still accessible through the remaining includes.

Suggested change
#include <ATen/native/xpu/sycl/Philox4x32.h>
#include <ATen/native/xpu/sycl/Philox4x32.h>
#include <ATen/native/xpu/sycl/DistributionTemplates.h>

Copilot uses AI. Check for mistakes.

auto rng_engine_inputs = PhiloxState(
std::get<0>(rng_engine_inputs_), std::get<1>(rng_engine_inputs_));
// Sample with replacement

Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment '// Sample with replacement' was removed but it provided valuable context for understanding the code section. Consider keeping meaningful comments that explain the algorithm or approach being used.

Suggested change
// Sample with replacement

Copilot uses AI. Check for mistakes.

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.

1 participant