[SYCL] [FPGA] Update handler of variadic template argument list in fpga_utils.hpp to support more types other than int32_t #3957

shuoniu-intel · 2021-06-18T18:01:28Z

This patch is a prerequisite for the upcoming FPGA latency control feature.

dm-vodopyanov

LGTM

bader · 2021-06-29T10:37:34Z

to support more types other than int32_t

Is there any test checking for that?

shuoniu-intel · 2021-06-29T12:42:36Z

to support more types other than int32_t

Is there any test checking for that?

Not yet, the upcoming latency control header change will use other types. But as for this patch, there's no functional difference in fpga.lsu.hpp.
Do you think I should add test in this patch?

bader · 2021-06-29T12:52:08Z

Do you think I should add test in this patch?

Yes. Otherwise it's quite likely that this functionality might be unintentionally broken by future patches (e.g. code refactoring).

shuoniu-intel · 2021-06-29T12:54:55Z

Do you think I should add test in this patch?

Yes. Otherwise it's quite likely that this functionality might be unintentionally broken by future patches (e.g. code refactoring).

Sure, could you point me where to add the test? I knew there was a sycl/test/fpga_tests folder, but seems it's gone now.

bader · 2021-06-29T13:34:53Z

Here are common guidelines: https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md#tests-development
@intel/llvm-reviewers-runtime should be able to provide more detailed instructions how to write a test for this feature.

romanovvlad · 2021-06-30T13:38:50Z

Here are common guidelines: https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md#tests-development
@intel/llvm-reviewers-runtime should be able to provide more detailed instructions how to write a test for this feature.

It seems we need a test which verifies produced IR here. @sergey-semenov Could you please confirm?

sergey-semenov · 2021-07-01T10:01:44Z

Here are common guidelines: https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md#tests-development
@intel/llvm-reviewers-runtime should be able to provide more detailed instructions how to write a test for this feature.

It seems we need a test which verifies produced IR here. @sergey-semenov Could you please confirm?

I think using static_assert to verify the correctness of the variadic templates with the newly supported types would be preferable here (would save us a run of FileCheck).

shuoniu-intel · 2021-07-05T13:53:52Z

Here are common guidelines: https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md#tests-development
@intel/llvm-reviewers-runtime should be able to provide more detailed instructions how to write a test for this feature.

It seems we need a test which verifies produced IR here. @sergey-semenov Could you please confirm?

I think using static_assert to verify the correctness of the variadic templates with the newly supported types would be preferable here (would save us a run of FileCheck).

If the only test we need here is a cpp file that includes this header, where should I put the test file? I don't see the guidelines link above covers it.

bader · 2021-07-06T12:37:03Z

sycl/test directory is the right place.
sycl/test/basic_tests/id_ctad.cpp - good example of using static_assert.

shuoniu-intel · 2021-07-07T13:53:51Z

I have created a new test - sycl/test/fpga_tests/arbitrary_template_arg.cpp. It's not a static_assert check but a host run check.
However I'm not sure why Jenkins/Precommit test times out on Windows, I don't think it's related to my new commit.
(http://icl-jenkins.sc.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FPrecommit_Check_User/detail/Precommit_Check_User/7484/pipeline)

bader · 2021-07-07T14:29:43Z

I have created a new test - sycl/test/fpga_tests/arbitrary_template_arg.cpp. It's not a static_assert check but a host run check.
However I'm not sure why Jenkins/Precommit test times out on Windows, I don't think it's related to my new commit.
(http://icl-jenkins.sc.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FPrecommit_Check_User/detail/Precommit_Check_User/7484/pipeline)

I would prefer to have the test similar to sycl/test/basic_tests/id_ctad.cpp with static_assert checks.
According to my understanding, to validate the changes made by this patch there is no need to run the compiled test, so we can reduce the test time and make it more reliable.
Does it make sense?

shuoniu-intel · 2021-07-07T14:42:44Z

I would prefer to have the test similar to sycl/test/basic_tests/id_ctad.cpp with static_assert checks.
According to my understanding, to validate the changes made by this patch there is no need to run the compiled test, so we can reduce the test time and make it more reliable.
Does it make sense?

It makes sense to remove the compiled test :).

As for the test I added, I want to check both type support (int and enum class) and arbitrary order support of template argument list (users don't have to specify all template arguments, and template arguments can be specified in arbitrary order). A run check covers both ends.

sergey-semenov · 2021-07-07T15:40:38Z

It makes sense to remove the compiled test :).

As for the test I added, I want to check both type support (int and enum class) and arbitrary order support of template argument list (users don't have to specify all template arguments, and template arguments can be specified in arbitrary order). A run check covers both ends.

I'm not quire sure how the runtime check here does anything a compile-time check wouldn't be able to do. Something like this should check the same behaviour as what you have now.

template <int ExpectedIntValue, enum_class ExpectedEnumValue, class... _Params> void func {
  static_assert(sycl::INTEL::_GetValue<test_int<0>, _Params...>::value == ExpectedIntValue);
  static_assert(sycl::INTEL::_GetValue<test_enum_class<enum_class::first>, _Params...>::value == ExpectedEnumValue);
}

int main() {
  func<0, enum_class::first>();
  func<1, enum_class::first, test_int<1>>();
  ...
}

Apply static_assert compile-time check.

shuoniu-intel · 2021-07-07T18:37:18Z

Changed my test to use compile-time static_assert.

sycl/test/fpga_tests/arbitrary_template_arg.cpp

Patch intel#3957 introduces `_statically_coalesce_val` in fpga_lsu.cpp like this: ``` _GetValue<statically_coalesce_impl<1>, _mem_access_params...>::value; ``` During merge conflict resolution in intel#4014 it was change accidentally to ``` _GetValue<statically_coalesce_impl<0>, _mem_access_params...>::value; ``` Restoring the right value.

#4206) Patch #3957 introduces `_statically_coalesce_val` in fpga_lsu.cpp like this: ``` _GetValue<statically_coalesce_impl<1>, _mem_access_params...>::value; ``` During merge conflict resolution in #4014 it was change accidentally to ``` _GetValue<statically_coalesce_impl<0>, _mem_access_params...>::value; ``` Restoring the right value.

intel#4206) Patch intel#3957 introduces `_statically_coalesce_val` in fpga_lsu.cpp like this: ``` _GetValue<statically_coalesce_impl<1>, _mem_access_params...>::value; ``` During merge conflict resolution in intel#4014 it was change accidentally to ``` _GetValue<statically_coalesce_impl<0>, _mem_access_params...>::value; ``` Restoring the right value.

…ment list The previous patch intel#3957 was a prerequisite change for the upcoming FPGA latency control feature. Revert due to a late change to the proposed use model API.

…ment list (#4260) The previous patch #3957 was a prerequisite change for the upcoming FPGA latency control feature. Revert due to a late change to the proposed use model API.

Update fpga_utils.hpp

c7da99b

shuoniu-intel changed the title ~~Update fpga_utils.hpp~~ [SYCL] [FPGA] Update LSU builtins template to allow types other than int32_t Jun 18, 2021

shuoniu-intel changed the title ~~[SYCL] [FPGA] Update LSU builtins template to allow types other than int32_t~~ [SYCL] [FPGA] Update fpga_utils.hpp to support types other than int32_t in the variadic template list Jun 18, 2021

shuoniu-intel changed the title ~~[SYCL] [FPGA] Update fpga_utils.hpp to support types other than int32_t in the variadic template list~~ [SYCL] [FPGA] Update fpga_utils.hpp to support types other than int32_t in the variadic template argument list Jun 18, 2021

shuoniu-intel added 4 commits June 18, 2021 14:26

Update fpga_lsu.hpp

19000e8

Clang-format fpga_lsu.hpp

7bd756d

Clang-format fpga_utils.hpp

ab5a614

Clang-format fpga_utils.hpp

b6d2035

shuoniu-intel changed the title ~~[SYCL] [FPGA] Update fpga_utils.hpp to support types other than int32_t in the variadic template argument list~~ [SYCL] [FPGA] Update handler of variadic template argument list in fpga_utils.hpp to support more types other than int32_t Jun 18, 2021

shuoniu-intel added 2 commits June 18, 2021 15:07

Merge branch 'intel:sycl' into shuoniu-br-template-list

86c69e5

Update fpga_utils.hpp

5498834

shuoniu-intel marked this pull request as ready for review June 28, 2021 13:59

shuoniu-intel requested a review from a team as a code owner June 28, 2021 13:59

shuoniu-intel requested a review from dm-vodopyanov June 28, 2021 13:59

dm-vodopyanov previously approved these changes Jun 28, 2021

View reviewed changes

shuoniu-intel added 2 commits July 6, 2021 09:31

Merge branch 'intel:sycl' into shuoniu-br-template-list

573f3c9

Create arbitrary_template_arg.cpp

2b96d49

shuoniu-intel dismissed dm-vodopyanov’s stale review via 2b96d49 July 6, 2021 13:34

shuoniu-intel added 3 commits July 6, 2021 11:13

Update arbitrary_template_arg.cpp

17eac46

Update arbitrary_template_arg.cpp

fce6391

Previous tests timeout, rerun tests.

4f44868

shuoniu-intel requested a review from dm-vodopyanov July 7, 2021 13:50

Update arbitrary_template_arg.cpp

44005ae

Apply static_assert compile-time check.

dm-vodopyanov reviewed Jul 9, 2021

View reviewed changes

sycl/test/fpga_tests/arbitrary_template_arg.cpp Show resolved Hide resolved

dm-vodopyanov approved these changes Jul 9, 2021

View reviewed changes

bader merged commit 8636e75 into intel:sycl Jul 9, 2021

shuoniu-intel deleted the shuoniu-br-template-list branch July 9, 2021 14:15

dm-vodopyanov mentioned this pull request Jul 28, 2021

[SYCL] Restore the correct initialization for _statically_coalesce_val #4206

Merged

This was referenced Aug 4, 2021

[SYCL] [FPGA] Manually revert header change of variadic template argu… shuoniu-intel/llvm#4

Closed

[SYCL] [FPGA] Manually revert header change of variadic template argument list #4260

Merged

[SYCL] [FPGA] Update handler of variadic template argument list in fpga_utils.hpp to support more types other than int32_t #3957

[SYCL] [FPGA] Update handler of variadic template argument list in fpga_utils.hpp to support more types other than int32_t #3957

Uh oh!

Conversation

shuoniu-intel commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dm-vodopyanov left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Jun 29, 2021

Uh oh!

shuoniu-intel commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader commented Jun 29, 2021

Uh oh!

shuoniu-intel commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader commented Jun 29, 2021

Uh oh!

romanovvlad commented Jun 30, 2021

Uh oh!

sergey-semenov commented Jul 1, 2021

Uh oh!

shuoniu-intel commented Jul 5, 2021

Uh oh!

bader commented Jul 6, 2021

Uh oh!

shuoniu-intel commented Jul 7, 2021

Uh oh!

bader commented Jul 7, 2021

Uh oh!

shuoniu-intel commented Jul 7, 2021

Uh oh!

sergey-semenov commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shuoniu-intel commented Jul 7, 2021

Uh oh!

Uh oh!

Uh oh!

shuoniu-intel commented Jun 18, 2021 •

edited

Loading

shuoniu-intel commented Jun 29, 2021 •

edited

Loading

shuoniu-intel commented Jun 29, 2021 •

edited

Loading

sergey-semenov commented Jul 7, 2021 •

edited

Loading