Skip to content

Conversation

terdner
Copy link
Contributor

@terdner terdner commented Aug 12, 2020

Description

Initial pull request for openMP_reductino and dpc_reduce examples.

Fixes # (issue)

Type of change

Please delete options that are not relevant. Add a 'X' to the one that is applicable.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Sample Migration (Moving sample from old repository after completing checklist established)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Command Line
  • oneapi-cli
  • Visual Studio
  • Eclipse IDE
  • VSCode

Checklist for Moving samples:

Links and Details can be found in the samples WG Teams Files.

@anjgola anjgola requested review from a team, JoeOster, akertesz, pmpeter1 and praveenkk123 and removed request for a team August 12, 2020 15:08
Copy link
Contributor

@JoeOster JoeOster left a comment

Choose a reason for hiding this comment

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

Here are a few things that need to be fixed

  • chrono commands should go through dpc_common
  • The use of '\n' is recommended over std::endl except if the explicit flush is desired
  • Needs a License.txt file in the root of both

samples

…still had <chrono> and the omp_common.hpp file. They have been removed.

Signed-off-by: todd.erdner <[email protected]>
…ed all calls to "std::endl" and replaced with " \n"

Signed-off-by: todd.erdner <[email protected]>
pmpeter1
pmpeter1 previously approved these changes Aug 12, 2020
JoeOster
JoeOster previously approved these changes Aug 12, 2020
@JoeOster
Copy link
Contributor

Test execution status

See detailed test results for specific OS below.

@mkitez - citests are in place, but Linux didn't run. I also don't see the report in TeamCity

Signed-off-by: todd.erdner <[email protected]>
Signed-off-by: todd.erdner <[email protected]>
praveenkk123
praveenkk123 previously approved these changes Aug 13, 2020
Copy link
Contributor

@praveenkk123 praveenkk123 left a comment

Choose a reason for hiding this comment

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

Trigger CI

@anjgola
Copy link
Contributor

anjgola commented Aug 17, 2020

@JoeOster Can you please review as well. @praveenkk123 If this meets all the checklist please click on 'merge and squash' and go ahead and merge it.

Copy link
Contributor

@JoeOster JoeOster left a comment

Choose a reason for hiding this comment

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

Need to fix the License.txt files, by removing date

@terdner
Copy link
Contributor Author

terdner commented Aug 18, 2020

@JoeOster - removed dates from the License.txt file and pushed to master.

JoeOster
JoeOster previously approved these changes Aug 18, 2020
@praveenkk123
Copy link
Contributor

@pmpeter1 , Hi Paul can you please confirm if you have reviewed and you are OK with these samples

Copy link

@pmpeter1 pmpeter1 left a comment

Choose a reason for hiding this comment

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

The dpc_common includes need the comments about how to find this shared file.

Would be good to rename the functions in the DPC++ sample to also include the "purpose" of the functions in the name and not just the execution mode like is done in the OpenMP sample.

@JoeOster
Copy link
Contributor

Here are a few things that need to be fixed

  • chrono commands should go through dpc_common
  • The use of '\n' is recommended over std::endl except if the explicit flush is desired
  • Needs a License.txt file in the root of both

samples

These issues were resolved

…i_*) per suggestion from Paul.

Signed-off-by: todd.erdner <[email protected]>
@terdner
Copy link
Contributor Author

terdner commented Aug 18, 2020

The dpc_common includes need the comments about how to find this shared file.

Would be good to rename the functions in the DPC++ sample to also include the "purpose" of the functions in the name and not just the execution mode like is done in the OpenMP sample.

@pmpeter1 - these two items are now fixed.

@JoeOster JoeOster merged commit 905fc5d into oneapi-src:master Aug 18, 2020
Copy link

@pmpeter1 pmpeter1 left a comment

Choose a reason for hiding this comment

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

Looks good.

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.

6 participants