Skip to content

Conversation

Summary: Refactor generate_kernel_call to support codegen call to Triton
kernel

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented May 22, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/102018

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 283f4f5:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

def generate_kernel_call(self, name, call_args, device_index=None, cpp=False):
self.writeline(self.wrap_kernel_call(name, call_args))
def generate_kernel_call(
self, name, call_args, grid=None, device_index=None, cuda=None
Copy link
Collaborator

Choose a reason for hiding this comment

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

what type is cuda? You are passing cuda=True, so default should probably be False, not None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it to cuda=True as the triton backend is the default one.

self, name: str, kernel: str, metadata: Optional[str] = None, cuda=True
):
if cpp:
if not cuda:
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the reason for changing arg from cpp to cuda?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason is because cpp means two things in inductor's codegen, the cpu backend and the cpp wrapper codegen. Because I added generate_kernel_call for the triton backend into the base class here, WrapperCodeGen, now we need a flag to distinguish the two backends, and cpp wouldn't be a good naming here. cuda may not be the best naming, but it's slightly better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@desertfire , I'm trying to enable Inductor to support other backends at runtime #100706. The design assumes the backend just needs to customize two meta classes, BaseScheduling and wrapper.WrapperCodeGen, by overriding some member functions. Regarding the define_kernel, I'm kind of confused by the cuda parameter because it is the logic of CudaWrapperCodeGen and it has implied cuda is True. My question is this parameter is must-have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I didn't have cuda as a parameter, but the problem appears when we have a mix of devices (see test_multi_device). Because we only keep one wrapper codegen instance, i.e. CudaWrapperCodeGen when there is a mix of cuda and cpu tensors, we need this additional parameter to help generate correct kernel definition and kernel call.

Summary: Refactor generate_kernel_call to support codegen call to Triton
kernel

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10

[ghstack-poisoned]
@desertfire
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 23, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants