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] Fix edge_detection and add it to the lit test-suite #87

Merged
merged 1 commit into from
Oct 1, 2020

Conversation

Ralender
Copy link
Contributor

edge_detection was failing because of the first argument was expected of xilinx_partition_array was in the generic address space instead of the default/stack address space.
I also added edge_detection to the test-suite it will run only if opencv4 is found and will only compile in hw_emu because images are too large datasets.

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

I have the feeling that now array partitioning is no longer used by the SYCL backend.
Note that it does not really change since it was ignored by HLS before... :-)

@@ -92,10 +101,10 @@ int main(int argc, char* argv[]) {
// to parallel_fors with local sizes
[=]() {
auto gX = xilinx::partition_array<char, 9,
xilinx::partition::complete<0>>({-1, 0, 1, -2, 0, 2, -1, 0, 1});
xilinx::partition::complete<1>>({-1, 0, 1, -2, 0, 2, -1, 0, 1});
Copy link
Member

Choose a reason for hiding this comment

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

Curious this change.
Are you changing the API?
Do you think it is better to start at 1?

@@ -258,13 +266,25 @@ def getDeviceCount(device_type):
if xocc == "only":
config.name = 'SYCL-XOCC'
config.test_source_root = config.test_source_root + "/xocc_tests"
run_if_hw="echo"
Copy link
Member

Choose a reason for hiding this comment

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

At least add some motivating comments

@@ -59,7 +53,7 @@ void dataflow(T functor) {
pipeline way.
*/
template <typename T>
SYCL_DEVICE_ANNOTATE("xilinx_pipeline") __attribute__((always_inline))
__SYCL_DEVICE_ANNOTATE("xilinx_pipeline") __attribute__((always_inline))
Copy link
Member

Choose a reason for hiding this comment

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

Could even be __TRISYCL prefix to reduce conflicts with Intel or other SYCL implementations

@@ -348,78 +348,6 @@ struct InSPIRation : public ModulePass {
}
}

/// This currently looks through the arguments passed to the SSDM intrinsic
Copy link
Member

Choose a reason for hiding this comment

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

So you are removing the support for the old style SSDM?
I guess it is no longer used anyway @yu810226 @aisoard
By the way, I am expecting the same level of comments elsewhere in the code... :-)

@keryell keryell merged commit 22603c6 into triSYCL:sycl/unified/next Oct 1, 2020
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.

llvm/sycl/test/xocc_tests/disabled/edge_detection example does not work
2 participants