Skip to content

Conversation

@vmaksimo
Copy link
Contributor

No description provided.

MrSidims
MrSidims previously approved these changes Sep 25, 2019
Signed-off-by: Viktoria Maksimova <[email protected]>
@vmaksimo
Copy link
Contributor Author

Commits should be squashed before merge, now waiting for CI checks

@MrSidims
Copy link
Contributor

Commits should be squashed before merge, now waiting for CI checks

You can squash the commits without waiting for testing results.

@vmaksimo
Copy link
Contributor Author

Commits should be squashed before merge, now waiting for CI checks

You can squash the commits without waiting for testing results.

As I know, it can be done automatically when PR is merged.

@bader
Copy link
Contributor

bader commented Sep 27, 2019

Enable clang unroll attribute instead of pragma unroll

@vmaksimo, @MrSidims, could you provide more details on motivation for this change?
What is wrong with the pragma unroll?

Also it's stated that "pragma unroll" is supposed to be disabled, but I don't see tests for that. I think it's just unfortunate wording, right?

@MrSidims
Copy link
Contributor

Enable clang unroll attribute instead of pragma unroll

@vmaksimo, @MrSidims, could you provide more details on motivation for this change?
What is wrong with the pragma unroll?

Nothing wrong with the pragma, but making it CXX attributes makes it nicer (also aligns it with other fpga loop attributes, not making users to write mixed-up code, like:
#pragma unroll 4 [[intelfpga::ivdep]] for(auto V : Vars) {}

Also it's stated that "pragma unroll" is supposed to be disabled, but I don't see tests for that. I think it's just unfortunate wording, right?

Just an unfortunate word indeed.

@bader bader merged commit 2f1e243 into intel:sycl Sep 27, 2019
@bader
Copy link
Contributor

bader commented Sep 27, 2019

Nothing wrong with the pragma, but making it CXX attributes makes it nicer (also aligns it with other fpga loop attributes, not making users to write mixed-up code, like:
#pragma unroll 4 [[intelfpga::ivdep]] for(auto V : Vars) {}

Please, put this information to the commit message/PR description next time.

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.

4 participants