Skip to content

[SYCL][NFC] Code clean up (phase 4) revealed by self build. #2878

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.

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

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

zahiraam
Copy link
Contributor

@zahiraam zahiraam commented Dec 8, 2020

No description provided.

@@ -235,7 +235,7 @@ struct TformCmd {

#define CHECK_AND_EXIT(E) \
{ \
Error LocE = std::move(E); \
Error LocE = E; \
Copy link
Contributor

@kbobrovs kbobrovs Dec 9, 2020

Choose a reason for hiding this comment

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

aren't old and new variants equivalent (taking into account CHECK_AND_EXIT caller sites)? What problem does this change solve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the error I was trying to fix:
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(277,9): error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
CHECK_AND_EXIT(Cmd.takeError());
^
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(238,18): note: expanded from macro 'CHECK_AND_EXIT'
Error LocE = std::move(E);
^
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(288,5): error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
CHECK_AND_EXIT(makeUserError("no inputs"));
^
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(238,18): note: expanded from macro 'CHECK_AND_EXIT'
Error LocE = std::move(E);
^
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(303,5): error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
CHECK_AND_EXIT(Table.takeError());
^
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(238,18): note: expanded from macro 'CHECK_AND_EXIT'
Error LocE = std::move(E);
^
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(315,5): error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
CHECK_AND_EXIT(createFileError(Output, EC));
^
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(238,18): note: expanded from macro 'CHECK_AND_EXIT'
Error LocE = std::move(E);
^
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(319,5): error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
CHECK_AND_EXIT(createFileError(Output, Out.error()));
^
D:/IUSERS/zahiraam/xmain_ws/xmain_self_build/llvm/llvm/tools/file-table-tform/file-table-tform.cpp(238,18): note: expanded from macro 'CHECK_AND_EXIT'
Error LocE = std::move(E);
^
5 errors generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right. I was looking at the call sites that are changed and could not see the temp. I should've looked to all other call sites instead.

@bader bader requested a review from kbobrovs December 10, 2020 10:23
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -235,7 +235,7 @@ struct TformCmd {

#define CHECK_AND_EXIT(E) \
{ \
Error LocE = std::move(E); \
Error LocE = E; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right. I was looking at the call sites that are changed and could not see the temp. I should've looked to all other call sites instead.

@zahiraam
Copy link
Contributor Author

@smaslov-intel Can you please review? Thanks.

@premanandrao
Copy link
Contributor

@romanovvlad, is this something you can help merge? Thanks!

@romanovvlad romanovvlad merged commit b330d1d into intel:sycl Dec 16, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Dec 21, 2020
* upstream/sycl: (616 commits)
  [SYCL][L0] Implement robust error handling in level_zero plugin (intel#2870)
  [SYCL][NFC] Code clean up (phase 5) revealed by self build. (intel#2907)
  [Driver][NFC] Remove unused variable (intel#2908)
  [Github Action] Enable automatic sync for main branch from llvm-project to llvm (intel#2904)
  [ESIMD][NFC] Remove unnecessary macro checks (intel#2900)
  [SYCL] Fix handling of multiple usages of composite spec constants (intel#2894)
  [SYCL] Adjust parallel-for range global size to improve group size selection (intel#2703)
  [SYCL] Add template parameter support for no_global_work_offset attribute (intel#2839)
  [SYCL] Support LLVM FP intrinsic in llvm-spirv and FE (intel#2880)
  [SYCL]Link Libm FP64 SYCL device library by default (intel#2892)
  [SYCL][NFC] Code clean up (phase 4) revealed by self build. (intel#2878)
  [SYCL][NFC] Code clean up (phase 3) revealed by self build. (intel#2865)
  [SYCL] Fix backend selection for SYCL_DEVICE_TYPE=* (intel#2890)
  [SYCL] Fix spec constants support in integration header (intel#2896)
  [Driver] Update unbundling of offload libraries to use archive type (intel#2883)
  [SYCL][NFC] Clang format SYCL.cpp (intel#2891)
  [CODEOWNERS] Add code owners for DPC++ tools (intel#2884)
  [XPTIFW] Enable in-tree builds (intel#2849)
  [SYCL] Don't dump IR and dot files by default in the LowerWGScope pass (intel#2887)
  [SYCL] Use llvm-link's only-needed option to link device libs (intel#2783)
  ...
@zahiraam zahiraam deleted the code-cleanup-4 branch February 7, 2022 15:27
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