Skip to content

[SYCL][COMPAT] Fix error on headers, add helloworld test #18401

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

Open
wants to merge 3 commits into
base: sycl
Choose a base branch
from

Conversation

Ruyk
Copy link
Contributor

@Ruyk Ruyk commented May 10, 2025

This PR fixes an error on the SYCL compat headers that caused build to fail.
The error manifests when building the basic hello world from the documentation.
To prevent the basic hello world from failing again, this PR adds an e2e for the basic hello world
and updates the documentation to match the updated hello world source code.

@Ruyk Ruyk requested review from a team as code owners May 10, 2025 21:37
@Ruyk Ruyk requested a review from aelovikov-intel May 10, 2025 21:37
@Ruyk Ruyk self-assigned this May 10, 2025
@Ruyk Ruyk added the syclcompat Issues related to SYCLcompat label May 10, 2025
@Ruyk Ruyk requested a review from Copilot May 10, 2025 21:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a build error in the SYCL compat headers and adds an end-to-end test for the basic hello world example while updating the corresponding documentation.

  • Fixes header issues causing build failures in SYCLcompat.
  • Adds an e2e test (helloworld.cpp) to verify the hello world example.
  • Updates the documentation to reflect the changes in header includes and error checking.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
sycl/test-e2e/Compat/helloworld.cpp Adds an e2e test for the hello world example
sycl/include/syclcompat/traits.hpp Updates type alias to use correct item instantiation
sycl/doc/syclcompat/README.md Updates header include path and error checking logic

@Ruyk Ruyk temporarily deployed to WindowsCILock May 10, 2025 21:42 — with GitHub Actions Inactive
@Ruyk Ruyk temporarily deployed to WindowsCILock May 10, 2025 22:06 — with GitHub Actions Inactive
@Ruyk Ruyk temporarily deployed to WindowsCILock May 10, 2025 22:06 — with GitHub Actions Inactive
if (std::abs(h_Y[i] - h_expected[i]) >= 1e-6) {
std::cerr << "Mismatch at index " << i << ": expected " << h_expected[i]
<< ", but got " << h_Y[i] << std::endl;
exit(EXIT_FAILURE);
Copy link
Contributor

Choose a reason for hiding this comment

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

EXIT_FAILURE and EXIT_SUCCESS need the cstdlib header

syclcompat::free(d_Y);

return EXIT_SUCCESS;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing trailing newline

Suggested change
}
}

Comment on lines 70 to 71
.get_info<sycl::info::device::max_work_group_size>())
block_size = 16;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.get_info<sycl::info::device::max_work_group_size>())
block_size = 16;
.get_info<sycl::info::device::max_work_group_size>()) {
block_size = 16;
}

I suppose we allow single line bodies without braces, but this particular line is a bit difficult to read.

@@ -0,0 +1,136 @@
/***************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be moved into the syclcompat folder instead of Compat

Comment on lines 26 to 32
#include <sycl/sycl.hpp>
#include <syclcompat/syclcompat.hpp>

#include <cassert>
#include <iostream>

#include <sycl/sycl.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

sycl/sycl.hpp is included twice, should only be once.

Also the current guidelines are to include as little as needed in E2E tests, so not the full sycl.hpp and syclcompat.hpp, in order to reduce testing times. Though I guess this particular test could be an exception if it's supposed to be used as learning material.

Copy link
Contributor

@ldrumm ldrumm left a comment

Choose a reason for hiding this comment

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

I don't like this direction. It's just going to diverge very shortly. If you want tests to test examples in documentation, then the correct way to do this is with a tool that parses the documentation and then builds that code, or conversely preprocesses a documentation template to include an external source file.
As it stands this doesn't test the documentation. It tests another piece of code somewhere else in the repository.

Worse: There's no note in the actual test file to say that it needs to be kept in sync with the README so we're pretty much guaranteed divergence unless someone's very careful with the git log...
Which brings up the second point: this PR does not do what the description says. It's described as fixing a header error, but it's actually about testing example code in documentation

Please reconsider this patch.

assert(h_Y[i] - h_expected[i] < 1e-6);
if (std::abs(h_Y[i] - h_expected[i]) >= 1e-6) {
std::cerr << "Mismatch at index " << i << ": expected " << h_expected[i]
<< ", but got " << h_Y[i] << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::endl on stderr does a double flush. I know this is example code, but I think it's best to lead by example, so \n would be better.

Also, why the de-morgan change? What does that buy us?

Also, if you're getting rid of the assert, then remove the header.

@ldrumm
Copy link
Contributor

ldrumm commented May 12, 2025

The correct way to do this is with a tool that parses the documentation and then builds that code

#!/usr/bin/env python

import sys
import tempfile
import re
import subprocess

ASCIIDOC_HEADER_PATTERN = re.compile(
    r'^==+ example (?P<example_name>[0-9]+)\s*$',
    re.IGNORECASE | re.UNICODE
)


def parse_example_sections(lines):
    sections = {}

    i = -1
    while i < len(lines) - 1:
        i += 1
        if not (match := ASCIIDOC_HEADER_PATTERN.match(lines[i])):
            continue
        if lines[i + 1].strip() != '[source,c++]':
            continue
        if lines[i + 2].strip() != '----':
            continue
        start = i + 3
        i = start
        while i < len(lines) and lines[i].strip() != '----':
            i += 1
        end = i
        sections[match.groupdict()['example_name']] = lines[start:end]
    return sections


if __name__ == '__main__':
    import argparse
    parser = argparse.ArgumentParser()
    parser.add_argument(
        'infile', nargs='?',
        type=argparse.FileType('r'),
        default=sys.stdin

    )
    known_args, extra_args = parser.parse_known_args()
    examples = parse_example_sections(known_args.infile.readlines())

    for example, source in examples.items():
        source = ''.join(source)
        print(f"compiling example {example}:")
        print(source)
        print("-------------------------------------------")
        with tempfile.NamedTemporaryFile(suffix='.cpp', delete=False) as f:
            f.write(source.encode('utf-8'))
            f.flush()
            subprocess.check_call(['clang++', '-fsycl',
            '-fsycl-targets=amdgcn-amd-amdhsa',
            '-Xsycl-target-backend=amdgcn-amd-amdhsa', '--offload-arch=gfx1032',
            '--rocm-path=/home/luke/h/hip/install'] + extra_args + [f.name])
        subprocess.check_call(['./a.out'])
        print(f"{example} passed")

Here's one I wrote for local testing of example code in a sycl extension doc I worked on. Should be simple to adapt the idea to markdown

@Ruyk Ruyk temporarily deployed to WindowsCILock May 12, 2025 15:16 — with GitHub Actions Inactive
@Ruyk Ruyk temporarily deployed to WindowsCILock May 12, 2025 15:43 — with GitHub Actions Inactive
@Ruyk Ruyk temporarily deployed to WindowsCILock May 12, 2025 15:43 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
syclcompat Issues related to SYCLcompat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants