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

Enable end to end test with Linalg input and Vulkan backend #415

Merged
merged 40 commits into from
Nov 1, 2022

Conversation

silee2
Copy link
Contributor

@silee2 silee2 commented Oct 26, 2022

This PR enables the first end to end test targeting GPU.
It has multiple related sub parts.

  1. Extend insert-gpu-allocs, set-spirv-capabilities and set-spirv-abi-attrs to support both OpenCL and Vulkan SPIRV
  2. Change how insert-gpu-allocs, set-spirv-capabilities and set-spirv-abi-attrs passes gets registered.
  3. Add test support on Windows
  4. Extend imex-runner.py - Support comments in pass pipeline file, add options to print IR before and after passes, add support for mlir-vulkan-runner in addition to mlir-cpu-runner
  5. Other compile issues uncovered by building on Windows.
  6. Add end to end test case.
  7. Change some defaults for gpu to spirv conversion pass. Now spirv memory space mapping is enabled and targets OpenCL.
  8. OpenCL pipeline is not completely end to end but includes all passes implemented so far. Upcoming passes are also added to pass pipeline but commented out for now.

Please review these guidelines to help with the review process:

  • Have you provided a meaningful PR description?
  • Have you added a test, a reproducer, or a reference to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • If this PR is a work in progress, are you filing the PR as a draft?
  • Have you organized your commits logically and ensured each can be built by itself?

@silee2 silee2 requested a review from nbpatel October 26, 2022 01:45
@silee2 silee2 marked this pull request as ready for review October 26, 2022 21:27
@silee2
Copy link
Contributor Author

silee2 commented Oct 26, 2022

I'll be taking PTO on Thur 10/17, so response would be delayed.

Now memref's integer space id is converted to spirv opencl space by
default.
Fix bug in imex-runner.py. MLIR's textual pass pipeline is sensitive to
space for mulit-option passes. space is used as a separator.
Add gpu to spirv pass and gpu to gpux pass to OpenCL pass pipeline.
…pu-to-spirv.

Extend OpenCL pipeline to spirv serialization.
@silee2
Copy link
Contributor Author

silee2 commented Oct 26, 2022

CI seems to have some LLVM caching issue after LLVM SHA update. CI throws error that some files are not found.

@silee2 silee2 mentioned this pull request Oct 27, 2022
6 tasks
Copy link
Contributor

@fschlimb fschlimb left a comment

Choose a reason for hiding this comment

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

Nice!
A few more comments in the code would be helpful.

@fschlimb
Copy link
Contributor

@diptorupd some changes to the cmake machinery, maybe this also touches #289

@fschlimb
Copy link
Contributor

6. Add end to end test case.

end-to-end would start with PTensor :)

if (!mlir::gpu::GPUDialect::isKernel(gpuFunc) ||
gpuFunc->getAttr(attrName))
continue;
if (m_clientAPI == "opencl") {
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems only getEntryPointABIAttr argument differs between opencl and vulkan, can this code be restructured to avoid code duplication?

Copy link
Contributor

@Dewei-Wang-sh Dewei-Wang-sh left a comment

Choose a reason for hiding this comment

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

lgtm overall

@@ -270,18 +270,17 @@ static BodyType buildTrivial(::mlir::Type typ) {
builder.create<IOP>(loc, lhs, rhs).getResult());
return;
} else
assert("Found integer type but binary op not defined for integers" ==
nullptr);
assert(0 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

you can also do assert(!"something") or just llvm_unreachable("something")

@Hardcode84
Copy link
Contributor

Did it picked up cache from main? You could just change LLVM_CACHE_NUMBER to force cache rebuild

@silee2 silee2 merged commit a1c8eab into intel:refactor Nov 1, 2022
@fschlimb
Copy link
Contributor

fschlimb commented Nov 2, 2022

Did it picked up cache from main? You could just change LLVM_CACHE_NUMBER to force cache rebuild

Every branch has its own cache. So the CI will not reuse the cache of a given branch for the merge commit.
Only the cache from main can be used on other branches. That's why it is desirable to have main use the same LLVM SHA as refactor and its forks.
Changing the LLVM_CACHE_NUMBER will not help with this issue. It is only useful if you need to force a rebuild of the cache (for example because you think it's corrupted).

mshahneo pushed a commit to mshahneo/mlir-extensions that referenced this pull request May 1, 2024
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.

5 participants