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.

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

Port and run tests in python/test/unit/tools #2953

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

ESI-SYD
Copy link
Contributor

@ESI-SYD ESI-SYD commented Dec 6, 2024

All cases pass except for tools/test_aot.py::test_compile_link_matmul_no_specialization SKIPPED (FIXME: AssertionError on XPU -> #2967

Compile support

Link support

gen bin success, but x1 hangs

todo:segfault fix

fix

Enable

Clean
@ESI-SYD ESI-SYD linked an issue Dec 6, 2024 that may be closed by this pull request
@etiotto etiotto requested review from anmyachev and pbchekin December 6, 2024 21:25
scripts/test-triton.sh Outdated Show resolved Hide resolved
cwd=dir,
)
o_files = glob.glob(os.path.join(dir, "*.o"))
if is_cuda():
Copy link
Contributor

Choose a reason for hiding this comment

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

May be to minimize changes and simplify future potential merges

Suggested change
if is_cuda():
if is_xpu():
gen_kernel_library_xpu(dir, libname)

and keep the rest of the function as the original?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review. Changed in f6aaed5

python/test/unit/tools/test_aot.py Outdated Show resolved Hide resolved
python/test/unit/tools/test_aot.py Outdated Show resolved Hide resolved
python/test/unit/tools/test_aot.py Outdated Show resolved Hide resolved
python/triton/tools/disasm.py Outdated Show resolved Hide resolved
python/triton/tools/link.py Outdated Show resolved Hide resolved
python/triton/tools/link.py Outdated Show resolved Hide resolved
python/triton/tools/link.py Outdated Show resolved Hide resolved
python/triton/tools/link.py Outdated Show resolved Hide resolved
python/triton/tools/link.py Outdated Show resolved Hide resolved
@ESI-SYD ESI-SYD requested a review from pbchekin December 9, 2024 06:49
Copy link
Contributor

@etiotto etiotto left a comment

Choose a reason for hiding this comment

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

I think there are opportunities in this PR to remove some changes to make the code less divergent from upstream

python/setup.py Show resolved Hide resolved
python/test/unit/tools/test_aot.py Show resolved Hide resolved
python/test/unit/tools/test_aot.py Outdated Show resolved Hide resolved
python/test/unit/tools/test_aot.py Show resolved Hide resolved
python/triton/tools/link.py Outdated Show resolved Hide resolved
python/triton/tools/link.py Outdated Show resolved Hide resolved
@@ -132,27 +134,46 @@ def gen_signature(m):

# generate declarations of kernels with meta-parameter and constant values
def make_algo_decls(name: str, metas: Sequence[KernelLinkerMeta]) -> str:
return f"""
if is_cuda():
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the check is_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.

No, return wrong source string for xpu if removed.



# generate declarations of kernels with meta-parameter and constant values
def make_global_decl(meta: KernelLinkerMeta) -> str:
return f"""
if is_cuda():
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the check is_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.

same reason

@etiotto
Copy link
Contributor

etiotto commented Dec 10, 2024

@whitneywhtsang are you OK with these changes?

Copy link
Contributor

@whitneywhtsang whitneywhtsang left a comment

Choose a reason for hiding this comment

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

can the two new files be moved to third_party/intel?

@ESI-SYD
Copy link
Contributor Author

ESI-SYD commented Dec 11, 2024

can the two new files be moved to third_party/intel?

Done

@whitneywhtsang
Copy link
Contributor

can the two new files be moved to third_party/intel?

Done

Thanks, maybe cleaner to put them in a folder, like third_party/intel/tools?

@ESI-SYD ESI-SYD enabled auto-merge (squash) December 11, 2024 02:38
@ESI-SYD ESI-SYD disabled auto-merge December 11, 2024 07:26
Copy link
Contributor

@whitneywhtsang whitneywhtsang Dec 11, 2024

Choose a reason for hiding this comment

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

Can this change be upstreamed?

Copy link
Contributor Author

@ESI-SYD ESI-SYD Dec 12, 2024

Choose a reason for hiding this comment

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

I think we can try. The change puts the backend templates in the corresponding location instead of the public folder, looks reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, please give that a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Port and run tests in python/test/unit/tools
4 participants