Skip to content

[SYCL][NFC] Code cleanup revealed by self-build. #3230

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
Merged
merged 1 commit into from
Mar 1, 2021
Merged

[SYCL][NFC] Code cleanup revealed by self-build. #3230

merged 1 commit into from
Mar 1, 2021

Conversation

zahiraam
Copy link
Contributor

No description provided.

@zahiraam zahiraam requested review from bader, smaslov-intel and a team as code owners February 18, 2021 16:53
@bader bader requested a review from andykaylor February 18, 2021 16:56
@zahiraam zahiraam force-pushed the selfbuild-errors-5 branch 3 times, most recently from 2534628 to 15496e2 Compare February 18, 2021 17:12
smaslov-intel
smaslov-intel previously approved these changes Feb 18, 2021
View reviewed changes
@bader
Copy link
Contributor

bader commented Feb 18, 2021

I'm not sure about llvm/* changes. These are source code, which we use as-is from the llvm/llvm-project, so I think it would be better to fix these issue there to avoid merge conflicts. I'd like to hear from Andy about the right way to handle issues like this.

@zahiraam
Copy link
Contributor Author

I'm not sure about llvm/* changes. These are source code, which we use as-is from the llvm/llvm-project, so I think it would be better to fix these issue there to avoid merge conflicts. I'd like to hear from Andy about the right way to handle issues like this.

@bader You mean they should be put into llorg?

@bader
Copy link
Contributor

bader commented Feb 18, 2021

I'm not sure about llvm/* changes. These are source code, which we use as-is from the llvm/llvm-project, so I think it would be better to fix these issue there to avoid merge conflicts. I'd like to hear from Andy about the right way to handle issues like this.

@bader You mean they should be put into llorg?

I think so.

@andykaylor
Copy link
Contributor

Can you provide more information on what the LLVM changes are fixing? I thought the llvm.org code was kept warning clean.

I agree that it would be much better to make the changes in llvm.org, but without know the error they're addressing I'm not sure these changes are correct. There's another symbol, LLVM_ENABLE_DUMP, that is independent of NDEBUG. A common guard is "#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)", but I don't understand why exclusion is needed at all in these cases.

@zahiraam
Copy link
Contributor Author

Can you provide more information on what the LLVM changes are fixing? I thought the llvm.org code was kept warning clean.

I agree that it would be much better to make the changes in llvm.org, but without know the error they're addressing I'm not sure these changes are correct. There's another symbol, LLVM_ENABLE_DUMP, that is independent of NDEBUG. A common guard is "#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)", but I don't understand why exclusion is needed at all in these cases.

@andykaylor All the errors happened during release build with "unused variable/reference".

@zahiraam
Copy link
Contributor Author

Can you provide more information on what the LLVM changes are fixing? I thought the llvm.org code was kept warning clean.
I agree that it would be much better to make the changes in llvm.org, but without know the error they're addressing I'm not sure these changes are correct. There's another symbol, LLVM_ENABLE_DUMP, that is independent of NDEBUG. A common guard is "#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)", but I don't understand why exclusion is needed at all in these cases.

@andykaylor All the errors happened during release build with "unused variable/reference".

@zahiraam zahiraam closed this Feb 22, 2021
@zahiraam zahiraam reopened this Feb 22, 2021
andykaylor
andykaylor previously approved these changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@andykaylor andykaylor left a comment

Choose a reason for hiding this comment

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

lgtm

keryell
keryell reviewed Feb 23, 2021
View reviewed changes
sycl/plugins/level_zero/pi_level_zero.cpp
@@ -4797,6 +4797,8 @@ enqueueMemImageCommandHelper(pi_command_type CommandType, pi_queue Queue,

// TODO: Level Zero does not support row_pitch/slice_pitch for images yet.
// Check that SYCL RT did not want pitch larger than default.
(void)RowPitch;
(void)SlicePitch;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you do not use these variables, why not... removing them? :-)

Such as having in the function definition just:

                             size_t /* RowPitch */, size_t /* SlicePitch */,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are used below inside the ASSERT, inside the DEBUG flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. So probably your change is adequate.
I guess it will be ugly to add the name in the function definition according to the value of NDEBUG... :-)

sycl/source/detail/memory_manager.cpp
@@ -112,6 +112,9 @@ void *MemoryManager::allocateInteropMemObject(
ContextImplPtr TargetContext, void *UserPtr,
const EventImplPtr &InteropEvent, const ContextImplPtr &InteropContext,
const sycl::property_list &, RT::PiEvent &OutEventToWait) {
(void)TargetContext;
(void)UserPtr;
(void)InteropContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right about UserPtr. Thanks.

@zahiraam
Copy link
Contributor Author

Review please! Thanks.

@romanovvlad romanovvlad merged commit 51f22c4 into intel:sycl Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keryell keryell keryell left review comments

@smaslov-intel smaslov-intel smaslov-intel approved these changes

@andykaylor andykaylor andykaylor left review comments

@bader bader Awaiting requested review from bader

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zahiraam @bader @andykaylor @keryell @smaslov-intel @romanovvlad