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

[SYCL] Temporarily disable hier_par test #1253

Closed
wants to merge 1 commit into from
Closed

[SYCL] Temporarily disable hier_par test #1253

wants to merge 1 commit into from

Conversation

andykaylor
Copy link
Contributor

The CodeGenSYCL/hier_par.cpp test is exhibiting inconsistent behavior.
This patch marks that test as UNSUPPORTED. When hierarchical parallelism
is stabilized the test should be re-enabled.

Signed-off-by: Andy Kaylor andrew.kaylor@intel.com

The CodeGenSYCL/hier_par.cpp test is exhibiting inconsistent behavior.
This patch marks that test as UNSUPPORTED. When hierarchical parallelism
is stabilized the test should be re-enabled.

Signed-off-by: Andy Kaylor <andrew.kaylor@intel.com>
@andykaylor andykaylor requested review from bader and againull March 5, 2020 15:45
@bader
Copy link
Contributor

bader commented Mar 5, 2020

FYI: @againull is also making changes to this file here #1205.
I think we should apply both PRs. @againull, do you agree?

@andykaylor
Copy link
Contributor Author

FYI: @againull is also making changes to this file here #1205.
I think we should apply both PRs. @againull, do you agree?

Can my change just be included in #1205?

@againull
Copy link
Contributor

againull commented Mar 5, 2020

In #1205 I was trying to fix the test and your PR is disabling it, so looks like merging them doesn't make sense. I want to update #1205 by changing the test to feed IR to LowerWGScope pass and check output IR (it looks like opt tool is available during check-clang). This should solve the problem with non-stable IR coming from frontend. Let me upload this change today so there will be non need to disable this test.

P.S. Also I am considering to move this pass to llvm because clang probably is the wrong place for it.

@andykaylor
Copy link
Contributor Author

If #1205 fixes the test then my change can be discarded. I just wanted to avoid the unexpected pass that sometimes happens.

@bader
Copy link
Contributor

bader commented Mar 6, 2020

In #1205 I was trying to fix the test and your PR is disabling it, so looks like merging them doesn't make sense. I want to update #1205 by changing the test to feed IR to LowerWGScope pass and check output IR (it looks like opt tool is available during check-clang).

+1 for running isolated pass instead of clang.

This should solve the problem with non-stable IR coming from frontend. Let me upload this change today so there will be non need to disable this test.

P.S. Also I am considering to move this pass to llvm because clang probably is the wrong place for it.

+1. LLVM project is the right place for LLVM passes.

Agree with Andy, if #1205 fixes the test, there is not need to disable, but I still see unfixed bugs in the pass, so I'm not sure if the test is going to be fixed by #1205.

@andykaylor
Copy link
Contributor Author

In #1205 I was trying to fix the test and your PR is disabling it, so looks like merging them doesn't make sense. I want to update #1205 by changing the test to feed IR to LowerWGScope pass and check output IR (it looks like opt tool is available during check-clang). This should solve the problem with non-stable IR coming from frontend. Let me upload this change today so there will be non need to disable this test.

P.S. Also I am considering to move this pass to llvm because clang probably is the wrong place for it.

This should absolutely be moved. There shouldn't be any passes in the front end.

@bader
Copy link
Contributor

bader commented Mar 7, 2020

@againull, feel free to close this PR, if you think #1205 will address hier_par test failures.

@againull
Copy link
Contributor

I prepared PR where I moved this pass to llvm project and replaced hier_par test:
#1279

Closing this PR.

@againull againull closed this Mar 10, 2020
@andykaylor andykaylor deleted the private/akaylor/uxpass branch March 11, 2020 21:43
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.

3 participants