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.

Sign up for GitHub

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

Jump to bottom

Pathfinder SYCL and numba-dpex implementation. #303

Closed
wants to merge 1 commit into from
Closed

Pathfinder SYCL and numba-dpex implementation. #303

wants to merge 1 commit into from

Conversation

roxx30198
Copy link
Contributor

@roxx30198 roxx30198 commented Oct 2, 2023 •
edited
Loading

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred 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?

Added the rodinia benchmark suite for dpbench/benchmarks. Also includes the native sycl, numba_dpex and python implementation for all algorithms.
Blocked by: IntelPython/numba-dpex#1106

@roxx30198 roxx30198 marked this pull request as draft October 2, 2023 18:09
@roxx30198 roxx30198 force-pushed the pathfinder branch 4 times, most recently from 08452c3 to b75b8cf Compare October 11, 2023 05:13
ZzEeKkAa
ZzEeKkAa reviewed Oct 11, 2023
View reviewed changes
dpbench/config/reader.py Outdated Show resolved Hide resolved
ZzEeKkAa
ZzEeKkAa reviewed Oct 11, 2023
View reviewed changes
dpbench/configs/bench_info/rodinia/pathfinder.toml
cols=10
pyramid_height=1

[benchmark.parameters.M16Gb]
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be almost the same as M, but memory should fit in 16Gb. You can refer how much memory was used in report. If memory is not a bottle neck, then it should be the same as M

ZzEeKkAa
ZzEeKkAa reviewed Oct 11, 2023
View reviewed changes
dpbench/console/run.py Outdated
parser.add_argument(
"--rodinia",
action=argparse.BooleanOptionalAction,
default=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, we can include rodinia benchmarks to be executed by default. @diptorupd , what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about it, and I guess it is better leave it disabled for now. In this case we need to update CI to test this code. You need to check it in two places:

Copy link

Choose a reason for hiding this comment

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

@ZzEeKkAa Sure we can do it as a later step, but we should enable the rodhinia benchmark running in GitHub CI at some point in near future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I have added it as a part of the CI and is run using the build_and_run.yml inside the workflow.

ZzEeKkAa
ZzEeKkAa suggested changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@ZzEeKkAa ZzEeKkAa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me. Just left minor comments. Please add description to PR and we are ready to go. Once it is ready - make sure to rebase and mark PR as ready to review.

@diptorupd diptorupd requested a review from adarshyoga October 11, 2023 18:03
adarshyoga
adarshyoga suggested changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@adarshyoga adarshyoga left a comment

Choose a reason for hiding this comment

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

The numba_dpex_k impl does not seem to match the SYCL impl. I notice that the SYCL impl is using barriers and the numba-dpex impl does not have any calls to a barrier.

dpbench/benchmarks/rodinia/pathfinder/pathfinder_initialize.py Outdated
LOW, HIGH, (rows * cols), dtype=np.int64
), np.empty(cols, dtype=np.int64)

return (data, rows, cols, pyramid_height, result)
Copy link
Contributor

Choose a reason for hiding this comment

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

rows, cols and pyramid_height need not be returned from initialize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unnecessary returns from initialize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The numba_dpex implementation had to modified as the multiplication of range elements wasn't allowed. Even though the sycl implementation had "iteration" inside the loop, for pyramid height as 1, it will always run once so we can compare them. Also as we are only dealing with independent elements, we won't be needing any barriers in this case.

dpbench/benchmarks/rodinia/pathfinder/pathfinder_numba_dpex_k.py Outdated Show resolved Hide resolved
dpbench/benchmarks/rodinia/pathfinder/pathfinder_numba_dpex_k.py Outdated Show resolved Hide resolved
dpbench/benchmarks/rodinia/pathfinder/pathfinder_python.py Outdated Show resolved Hide resolved
dpbench/benchmarks/rodinia/pathfinder/pathfinder_numba_dpex_k.py Outdated Show resolved Hide resolved
adarshyoga
adarshyoga reviewed Oct 13, 2023
View reviewed changes
dpbench/benchmarks/rodinia/pathfinder/pathfinder_initialize.py Outdated
SEED = 9


def initialize(rows, cols, pyramid_height, types_dict=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

pyramid_height is not being used in initialization.

adarshyoga
adarshyoga reviewed Oct 13, 2023
View reviewed changes
dpbench/benchmarks/rodinia/pathfinder/pathfinder_numba_dpex_k.py Outdated
device_dest[current_element] += shortest


def pathfinder(data, rows, cols, pyramid_height, result):
Copy link
Contributor

Choose a reason for hiding this comment

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

pyramid_height not being used?

@roxx30198 roxx30198 force-pushed the pathfinder branch 2 times, most recently from 5a3d1ce to 01b6bc9 Compare October 16, 2023 22:20
@roxx30198 roxx30198 closed this Nov 8, 2023
@roxx30198 roxx30198 reopened this Nov 20, 2023
@roxx30198 roxx30198 marked this pull request as ready for review November 20, 2023 23:42
@roxx30198 roxx30198 force-pushed the pathfinder branch 4 times, most recently from 53de4a8 to bcd2a25 Compare November 21, 2023 18:55
@roxx30198 roxx30198 marked this pull request as draft November 22, 2023 16:45
@adarshyoga
Copy link
Contributor

Closing this PR. Please reopen if there are further updates.

@adarshyoga adarshyoga closed this Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adarshyoga adarshyoga adarshyoga requested changes

@diptorupd diptorupd diptorupd left review comments

@ZzEeKkAa ZzEeKkAa Awaiting requested review from ZzEeKkAa

Assignees

@roxx30198 roxx30198

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@roxx30198 @adarshyoga @diptorupd @ZzEeKkAa