Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[inductor] Fix CPP wrapper codegen for ExternKernel args #117931

Closed
wants to merge 1 commit into from
Closed

[inductor] Fix CPP wrapper codegen for ExternKernel args #117931

wants to merge 1 commit into from
+41 −3

Conversation

aakhundov
Copy link
Contributor

@aakhundov aakhundov commented Jan 20, 2024 •
edited

Summary: We see IR nodes repr-ed directly in the CPP wrapper codegen. Recently, this issue has been fixed for the Python wrapper codegen in D52899373 (#117838). Here we extend the fix to CPP wrapper codegen / AOTInductor.

Test Plan:
New unit tests. In OSS:

python test/inductor/test_aot_inductor.py -k test_triton_kernel_multi_output_arg

python test/inductor/test_aot_inductor.py -k test_triton_kernel_extern_kernel_arg

Differential Revision: D52936248

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Jan 20, 2024 •
edited

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit d64e7a6 with merge base 05ef203 (image):

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

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

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52936248

@aakhundov aakhundov added ciflow/trunk Trigger trunk jobs on your pull request topic: not user facing topic category labels Jan 20, 2024
aakhundov added a commit to aakhundov/pytorch that referenced this pull request Jan 20, 2024
@aakhundov @facebook-github-bot
[inductor] Fix CPP wrapper codegen for ExternKernel args (pytorch#117931
9a1d2f0 
)

Summary:

Recently, this has been fixed for the Python wrapper codegen in D52899373 (pytorch#117838). Here we extend the fix to CPP wrapper codegen / AOTInductor.

Test Plan:
New unit tests. In OSS:

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_multi_output_arg
```

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_extern_kernel_arg
```

Differential Revision: D52936248
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52936248

aakhundov added a commit to aakhundov/pytorch that referenced this pull request Jan 20, 2024
@aakhundov @facebook-github-bot
[inductor] Fix CPP wrapper codegen for ExternKernel args (pytorch#117931
719a71f 
)

Summary:

Recently, this has been fixed for the Python wrapper codegen in D52899373 (pytorch#117838). Here we extend the fix to CPP wrapper codegen / AOTInductor.

Test Plan:
New unit tests. In OSS:

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_multi_output_arg
```

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_extern_kernel_arg
```

Differential Revision: D52936248
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52936248

aakhundov added a commit to aakhundov/pytorch that referenced this pull request Jan 20, 2024
@aakhundov @facebook-github-bot
[inductor] Fix CPP wrapper codegen for ExternKernel args (pytorch#117931
34feaf8 
)

Summary:

Recently, this has been fixed for the Python wrapper codegen in D52899373 (pytorch#117838). Here we extend the fix to CPP wrapper codegen / AOTInductor.

Test Plan:
New unit tests. In OSS:

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_multi_output_arg
```

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_extern_kernel_arg
```

Reviewed By: oulgen

Differential Revision: D52936248
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52936248

@oulgen
Copy link
Contributor

oulgen commented Jan 20, 2024

I mentioned this in a direct message to Adnan but for posterity, it would be good to do a bit of a code clean up in wrapper.py. There are so many semi identical functions between jit inductor and aot inductor, val_to_arg_str being a prime suspect.

@aakhundov
Copy link
Contributor Author

I mentioned this in a direct message to Adnan but for posterity, it would be good to do a bit of a code clean up in wrapper.py. There are so many semi identical functions between jit inductor and aot inductor, val_to_arg_str being a prime suspect.

Yeah, the wrapper codegen refactoring is planned. cc @desertfire @chenyang78.

aakhundov added a commit to aakhundov/pytorch that referenced this pull request Jan 20, 2024
@aakhundov @facebook-github-bot
[inductor] Fix CPP wrapper codegen for ExternKernel args (pytorch#117931
d395df7 
)

Summary:

Recently, this has been fixed for the Python wrapper codegen in D52899373 (pytorch#117838). Here we extend the fix to CPP wrapper codegen / AOTInductor.

Test Plan:
New unit tests. In OSS:

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_multi_output_arg
```

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_extern_kernel_arg
```

Reviewed By: oulgen, chenyang78

Differential Revision: D52936248
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52936248

@aakhundov @facebook-github-bot
[inductor] Fix CPP wrapper codegen for ExternKernel args (pytorch#117931
d64e7a6 
)

Summary:

Recently, this has been fixed for the Python wrapper codegen in D52899373 (pytorch#117838). Here we extend the fix to CPP wrapper codegen / AOTInductor.

Test Plan:
New unit tests. In OSS:

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_multi_output_arg
```

```
python test/inductor/test_aot_inductor.py -k test_triton_kernel_extern_kernel_arg
```

Reviewed By: oulgen, chenyang78

Differential Revision: D52936248
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52936248

@chenyang78
Copy link
Contributor

I mentioned this in a direct message to Adnan but for posterity, it would be good to do a bit of a code clean up in wrapper.py. There are so many semi identical functions between jit inductor and aot inductor, val_to_arg_str being a prime suspect.

Yeah, wrapper.py is getting quite messy now. I will start refactoring it very soon, likely this coming week. Thanks.

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -f 'Landed internally'

(Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but landed internally)

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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
Reviewers

@chenyang78 chenyang78 chenyang78 approved these changes

@desertfire desertfire desertfire approved these changes

@oulgen oulgen oulgen approved these changes

Assignees
No one assigned
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request fb-exported Merged module: inductor topic: not user facing topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aakhundov @facebook-github-bot @oulgen @chenyang78 @pytorchmergebot @desertfire