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

Followup review feedback on e2e codegen example #876

Conversation

omarahmed1111
Copy link
Contributor

@omarahmed1111 omarahmed1111 commented Sep 20, 2023

This PR is a follow up PR to address the rest of review feedback on 610. It contains:

  • readme file to instruct how to run the codegen example.
  • filter out adapters that doesn't support spirv kernels from codegen example.
  • print the input array to the spv kernel in the codegen example.

Fixes: 859

@omarahmed1111
Copy link
Contributor Author

is this for page alignment?
getpagesize() ?

It would make since that this is related to gpus page size, but not sure how to make sure of that. I think it will need to be generalized if it will run on anything other than L0.

@omarahmed1111
Copy link
Contributor Author

omarahmed1111 commented Sep 20, 2023

Now that we have GPU-equipped runners, it should be possible to create a workflow where this example is built and run as part of tests. Otherwise, it will be easy to accidentally break it.

I would think that would still be blocked by this issue, so it will be hard to test that while failure, I think that could be added as a seperate PR with a seperate issue when the L0 pipeline is fixed.

@omarahmed1111 omarahmed1111 force-pushed the followup-review-feedback-on-e2e-codegen-example branch from 45f3701 to 905699f Compare September 20, 2023 15:56
@omarahmed1111 omarahmed1111 marked this pull request as ready for review September 20, 2023 16:00
examples/codegen/codegen.cpp Show resolved Hide resolved
examples/codegen/README.md Outdated Show resolved Hide resolved
@pbalcer
Copy link
Contributor

pbalcer commented Sep 21, 2023

is this for page alignment?
getpagesize() ?

It would make since that this is related to gpus page size, but not sure how to make sure of that. I think it will need to be generalized if it will run on anything other than L0.

I guess 4k is a safe enough value... I'd still do a define/const variable instead of having a magic number though.

@pbalcer
Copy link
Contributor

pbalcer commented Sep 21, 2023

Now that we have GPU-equipped runners, it should be possible to create a workflow where this example is built and run as part of tests. Otherwise, it will be easy to accidentally break it.

I would think that would still be blocked by this issue, so it will be hard to test that while failure, I think that could be added as a seperate PR with a seperate issue when the L0 pipeline is fixed.

The L0 workflow is not blocked, that test is simply not ran in CI (plus there's a timeout). But I agree that it might make sense to do this in another PR.

examples/codegen/README.md Outdated Show resolved Hide resolved
examples/codegen/README.md Outdated Show resolved Hide resolved
examples/codegen/README.md Show resolved Hide resolved
@omarahmed1111 omarahmed1111 force-pushed the followup-review-feedback-on-e2e-codegen-example branch 4 times, most recently from b418b8d to 05d7bb9 Compare September 21, 2023 13:38
@omarahmed1111
Copy link
Contributor Author

Now that we have GPU-equipped runners, it should be possible to create a workflow where this example is built and run as part of tests. Otherwise, it will be easy to accidentally break it.

I would think that would still be blocked by this issue, so it will be hard to test that while failure, I think that could be added as a seperate PR with a seperate issue when the L0 pipeline is fixed.

The L0 workflow is not blocked, that test is simply not ran in CI (plus there's a timeout). But I agree that it might make sense to do this in another PR.

I created this issue to track this. I was trying to attach it to this pr but it seems that it will not work directly and might be controversial in some parts. so, will submit it to a new pr.

@omarahmed1111 omarahmed1111 force-pushed the followup-review-feedback-on-e2e-codegen-example branch from 05d7bb9 to 398e8b1 Compare September 21, 2023 13:55
examples/codegen/codegen.cpp Outdated Show resolved Hide resolved
@omarahmed1111 omarahmed1111 force-pushed the followup-review-feedback-on-e2e-codegen-example branch from 398e8b1 to bceddab Compare September 22, 2023 13:53
Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

👍

@omarahmed1111 omarahmed1111 force-pushed the followup-review-feedback-on-e2e-codegen-example branch from bceddab to d2edad6 Compare September 26, 2023 13:32
@omarahmed1111 omarahmed1111 merged commit 7180719 into oneapi-src:adapters Sep 27, 2023
36 checks passed
@omarahmed1111 omarahmed1111 deleted the followup-review-feedback-on-e2e-codegen-example branch September 27, 2023 10:13
@omarahmed1111 omarahmed1111 linked an issue Sep 27, 2023 that may be closed by this pull request
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.

Followup review feedback on e2e codegen example
5 participants