[SYCL][ABI-break] Remove <iostream> headers from "sycl/sycl.hpp" and related files #6337

raaiq1 · 2022-06-21T23:58:07Z

Refactored code base to remove dependency from "sycl/sycl.hpp". The main motivation behind it is so confrom by LLVM coding standards (https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden) which forbids the inclusion of <iostream> in library files. This required the following:

Replacing cout or cerr or clog statements with fprintf() statements
Removing >> , << operators for sycl classes that interact with <iostream> and substituting them with std::string cast operator if possible, otherwise a to_string method was defined (as the case in backend enum)
Removing any calls to the operators mentioned above (if a string::cast operator couldn't be defined) and replacing them with printf
Refactoring some class methods to take a FILE* instead of an <iostream> object, such as void DeviceBinaryImage::dump(std::ostream &Out) const under ''sycl/include/CL/sycl/detail/pi.hpp" and virtual void Command::printDot(std::ostream &Stream) const = 0; and it's various overloads

…ream> in headers

v

Signed-off-by: Rauf, Rana <[email protected]>

g

sycl/include/CL/sycl/backend_types.hpp

sycl/include/CL/sycl/detail/device_filter.hpp

sycl/include/CL/sycl/detail/pi.hpp

sycl/include/sycl/ext/intel/esimd/detail/types.hpp

sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp

sycl/plugins/hip/pi_hip.cpp

sycl/plugins/level_zero/usm_allocator.cpp

sycl/source/detail/plugin_printers.hpp

sycl/plugins/level_zero/usm_allocator.cpp

sycl/source/detail/config.hpp

sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp

Signed-off-by: Rauf, Rana <[email protected]>

c

…s unittest Signed-off-by: Rauf, Rana <[email protected]>

raaiq1 · 2022-07-14T19:20:51Z

/verify with intel/llvm-test-suite#1084

raaiq1 · 2022-07-14T19:58:37Z

/verify with intel/llvm-test-suite#1084

Signed-off-by: Rauf, Rana <[email protected]>

raaiq1 · 2022-07-15T15:56:10Z

/verify with intel/llvm-test-suite#1084

pull

raaiq1 · 2022-07-15T17:37:15Z

/verify with intel/llvm-test-suite#1084

cperkinsintel · 2022-07-18T15:18:54Z

/verify with intel/llvm-test-suite#1084

Signed-off-by: Rauf, Rana <[email protected]>

cperkinsintel · 2022-07-19T17:07:24Z

/verify with intel/llvm-test-suite#1084

cperkinsintel

LGTM.

cperkinsintel · 2022-07-20T17:44:05Z

/verify with intel/llvm-test-suite#1084

cperkinsintel · 2022-07-20T23:49:23Z

ping to other reviewers. The 'Jenkins/llvm-test-suite' is passing against the matching PR intel/llvm-test-suite#1084 on the test suite.

cperkinsintel · 2022-07-21T15:37:41Z

ping to other reviewers. I know this touches a lot of files, which makes the review a bit of a chore. But also the longer this takes to review and merge, the more likely there will be merge conflicts that'll just set the reviews back. The llvm-test-suite has been updated to match intel/llvm-test-suite#1084 and the "Jenkins/llvm-test-suite" entry in the checks above demonstrates that these are passing.

bader · 2022-07-21T18:12:12Z

sycl/include/sycl/detail/common.hpp

@@ -116,14 +116,16 @@ static inline std::string codeToString(pi_int32 code) {
                          "Native API returns: "

 #ifndef __SYCL_SUPPRESS_PI_ERROR_REPORT
-#include <iostream>
+#include <cstdio>


@raaiq1, please, add motivation for this change to the PR description. Right now it just describes what PR does, but doesn't explain why do we need this change.

I guess performance has something to do with it. Am I right?
FYI: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slio3-prefer-iostreams-for-io

The main motivation behind it to abide by llvm coding standards, in this case it's avoid including <iostream> in library files: https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden

Please, add such information to the pull request description. It's important context information for doing code review.

bader · 2022-07-21T18:16:46Z

sycl/include/sycl/detail/device_filter.hpp

+  std::string Out{};
+  Out += backend_to_string(this->Backend);


Suggested change

std::string Out{};

Out += backend_to_string(this->Backend);

std::string Out = backend_to_string(this->Backend);

bader · 2022-07-21T18:26:54Z

sycl/include/sycl/detail/device_filter.hpp

+  std::string Out{};
+  Out += backend_to_string(this->Backend);
+  Out += ":";
+  switch (this->DeviceType) {


Just out of curiosity: is this-> really needed here?

bader · 2022-07-21T18:28:53Z

sycl/plugins/cuda/pi_cuda.cpp

+  std::string versionString;
+  versionString = "CUDA ";


Suggested change

std::string versionString;

versionString = "CUDA ";

std::string versionString = "CUDA ";

bader · 2022-07-21T18:31:23Z

sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp

@@ -114,7 +115,8 @@ static bool PrintPiTrace = false;

 static void PiTrace(std::string TraceString) {
  if (PrintPiTrace) {
-    std::cout << TraceString << std::endl;
+    printf("%s\n", TraceString.c_str());
+    fflush(stdout);


Why do we need to change anything in source files? The PR title says is removed from headers files.

NOTE: in addition to source files we can keep plug-in header files untouched, if we update only header files included into sycl/sycl.hpp.
The same applies to all headers in source directory. They are not included to sycl/sycl.hpp as well.

I'm sorry the PR title is a little misleading. This does remove <iostream> headers form the source files as well. So according to LLVM coding standards <iostream> headers are forbidden in 'library files' (https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden). I interpreted 'library files' to include both the headers and source files. I might be wrong though

bader · 2022-07-21T18:39:45Z

sycl/test/helpers.hpp

+#include <iostream>
 #include <sycl/sycl.hpp>


Suggested change

#include <iostream>

#include <sycl/sycl.hpp>

#include <sycl/sycl.hpp>

#include <iostream>

Let's include iostream after sycl.hpp to catch cases when sycl.hpp uses iostream functionality w/o including the header.

bader · 2022-07-21T18:40:29Z

sycl/test/gdb/iostream_test.cpp

@@ -0,0 +1,13 @@
+// UNSUPPORTED: true


Why is it here?

bader · 2022-07-21T18:44:22Z

sycl/test/gdb/iostream_test.cpp

+// RUN: gdb %t.out -batch -x %sycl_gdb_iostream 2>&1 | FileCheck %s
+// CHECK: ignore next 9998 hits
+//
+// Tests if <sycl/sycl.hpp> headers include any <iostream> headers


I think it can be done w/o running a GDB, but just checking the compiler output.
NOTE: it checks host side of the compilation only, so you don't need to compile with -fsycl option.

bader · 2022-07-21T18:50:03Z

sycl/source/detail/config.cpp

+  if (c == '\n') {
+    return currentIndex;
+  }


The code above doesn't use braces for one-statement branches. Please, use consistent code style.

Suggested change

if (c == '\n') {

return currentIndex;

}

if (c == '\n')

return currentIndex;

raaiq1 · 2022-07-22T21:34:57Z

@bader @cperkinsintel @kbobrovs There is an alternative solution @aelovikov-intel suggested. I've made a draft PR for it (#6469). They both accomplish the same goal but this PR may have some performance improvements but I'm not sure. I would like to discuss if it's better to merge this PR or the draft PR

bader · 2022-07-24T14:29:01Z

@bader @cperkinsintel There is an alternative solution @aelovikov-intel suggested. I've made a draft PR for it (#6469). They both accomplish the same goal but this PR may have some performance improvements but I'm not sure. I would like to discuss if it's better to merge this PR or the draft PR

I think #6469 might be good enough as first step, but eventually it would be great to have some soft of internal logger, which can isolate C or C++ APIs we use to output error/warning/notification information.

#include "dpcpp_logger.h"

ERR(<args>);                  // resolves into std::cerr/clog or fprintf(stderr, ...)
INFO(<args>)/WARN(<args>);    // resolves into std::cout or fprintf(stdout, ...)

I think this can be done with zero run-time overhead (i.e. can as fast as using std::cerr/cout or fprintf directly) and simplify maintenance.

kbobrovs · 2022-07-25T00:52:00Z

sycl/include/sycl/ext/intel/esimd/simd.hpp

  /// Prefix increment, increments elements of this object.
  /// @return Reference to this object.
-  simd &operator++() {
+  simd &


please drop this change

kbobrovs · 2022-07-25T01:15:31Z

sycl/include/sycl/ext/intel/esimd/simd.hpp

@@ -125,9 +125,30 @@ class simd : public detail::simd_obj_impl<
    return sycl::ext::oneapi::experimental::simd<Ty, N1>(base_type::data());
  }

+  /// @ingroup sycl_esimd_misc


this is wrong doc group. just drop it

kbobrovs · 2022-07-25T01:15:53Z

sycl/include/sycl/ext/intel/esimd/simd.hpp

@@ -125,9 +125,30 @@ class simd : public detail::simd_obj_impl<
    return sycl::ext::oneapi::experimental::simd<Ty, N1>(base_type::data());
  }

+  /// @ingroup sycl_esimd_misc
+  /// Prints a \c simd object to an output stream.


this is not what the code is doing now, it rather converts the object to std::string

kbobrovs · 2022-07-25T01:30:11Z

sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp

-    sprintf(ESimdEmuVersionString, "0.%d.%d", ESIMDEmuPluginInterfaceVersion,
-            ESIMDEmuPluginDataVersion);
+    sprintf(ESimdEmuVersionString, "0.%lld.%lld",
+            (long long int)ESIMDEmuPluginInterfaceVersion,


why this change is needed? the *Version are #define's, so %d should work OK - ?

kbobrovs · 2022-07-25T01:31:42Z

sycl/source/esimd_emulator_device_interface.cpp

+            "%s\n"
+            "The device interface version provided from plug-in"
+            "library is behind required device interface version\n"
+            "Found version : %llu\n"


%llu does not seem to be right for printing uintptr_t. See e.g. https://stackoverflow.com/questions/5795978/string-format-for-intptr-t-and-uintptr-t

raaiq1 · 2022-07-27T17:43:59Z

This PR is being closed, as it touches a lot of files and prone to more errors. An alternative PR has been opened for the simpler solution (#6469)

raaiq1 and others added 5 commits June 17, 2022 10:32

replaced cout|cin|clog with respective c counterparts to remove <iost…

3fe426b

…ream> in headers

Merge pull request #2 from intel/sycl

58ebf67

v

remoeved iostream from sycl headers

f2b5f89

Signed-off-by: Rauf, Rana <[email protected]>

Merge branch 'sycl' into raaiq

1ba59dd

small chagnes

780cb9c

raaiq1 requested review from a team as code owners June 21, 2022 23:58

raaiq1 requested a review from steffenlarsen June 21, 2022 23:58

Removed unnecessary comments

60cb520

raaiq1 force-pushed the raaiq branch from 9c90bf1 to 60cb520 Compare June 22, 2022 17:00

raaiq1 added 2 commits June 22, 2022 13:06

Merge pull request #4 from intel/sycl

06f3856

g

Merge branch 'sycl' into raaiq

96ff86e

steffenlarsen reviewed Jun 22, 2022

View reviewed changes

cperkinsintel self-requested a review June 24, 2022 19:31

cperkinsintel reviewed Jun 24, 2022

View reviewed changes

sycl/plugins/level_zero/usm_allocator.cpp Outdated Show resolved Hide resolved

cperkinsintel reviewed Jun 24, 2022

View reviewed changes

sycl/source/detail/config.hpp Outdated Show resolved Hide resolved

cperkinsintel reviewed Jun 24, 2022

View reviewed changes

sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp Outdated Show resolved Hide resolved

raaiq1 marked this pull request as draft June 24, 2022 21:51

raaiq1 and others added 10 commits June 27, 2022 08:21

Addressing PR issues

1f63594

Signed-off-by: Rauf, Rana <[email protected]>

next pr

de8792b

Signed-off-by: Rauf, Rana <[email protected]>

format changes

d64275a

more clang-format changes

3a66ee4

Merge branch 'raaiq' of https://github.com/raaiq1/llvm into raaiq

8ce2fe9

thought formatting was done

63e2155

Merge pull request #5 from intel/sycl

6c85afe

c

fixed compilation errors

3104b5f

fixed bugs and errors on previous changes

786b793

Refectored persistent_device_code_cache.cpp to exclude iostream, fail…

8c49996

…s unittest Signed-off-by: Rauf, Rana <[email protected]>

More format changes

71ea7ed

message

52a45da

raaiq1 added 2 commits July 15, 2022 07:18

Casted all intergral types to long long for portability

1f6df1b

Signed-off-by: Rauf, Rana <[email protected]>

Marked gdb counter test unsupported and moved test to better directory

4deaffd

Signed-off-by: Rauf, Rana <[email protected]>

Merge pull request #11 from intel/sycl

8657632

pull

raaiq1 and others added 3 commits July 18, 2022 10:54

Whats up

d890d56

Signed-off-by: Rauf, Rana <[email protected]>

Merge branch 'raaiq' of https://github.com/raaiq1/llvm into raaiq

31cf873

Merge branch 'intel:sycl' into raaiq

171094b

raaiq1 changed the title ~~[SYCL][ABI-break] Remove <iostream> headers from "CL/sycl.hpp" and related files~~ [SYCL][ABI-break] Remove <iostream> headers from "sycl/sycl.hpp" and related files Jul 19, 2022

cperkinsintel approved these changes Jul 20, 2022

View reviewed changes

Merge branch 'intel:sycl' into raaiq

dc561f7

smaslov-intel approved these changes Jul 21, 2022

View reviewed changes

bader reviewed Jul 21, 2022

View reviewed changes

Merge branch 'intel:sycl' into raaiq

76cfc00

kbobrovs reviewed Jul 25, 2022

View reviewed changes

raaiq1 marked this pull request as draft July 25, 2022 20:45

raaiq1 closed this Jul 27, 2022

raaiq1 deleted the raaiq branch August 2, 2022 17:17

	std::string Out{};
	Out += backend_to_string(this->Backend);
	std::string Out = backend_to_string(this->Backend);

	std::string versionString;
	versionString = "CUDA ";
	std::string versionString = "CUDA ";

[SYCL][ABI-break] Remove <iostream> headers from "sycl/sycl.hpp" and related files #6337

[SYCL][ABI-break] Remove <iostream> headers from "sycl/sycl.hpp" and related files #6337

Uh oh!

Conversation

raaiq1 commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raaiq1 commented Jul 14, 2022

Uh oh!

raaiq1 commented Jul 14, 2022

Uh oh!

raaiq1 commented Jul 15, 2022

Uh oh!

raaiq1 commented Jul 15, 2022

Uh oh!

cperkinsintel commented Jul 18, 2022

Uh oh!

cperkinsintel commented Jul 19, 2022

Uh oh!

cperkinsintel left a comment

Choose a reason for hiding this comment

Uh oh!

cperkinsintel commented Jul 20, 2022

Uh oh!

cperkinsintel commented Jul 20, 2022

Uh oh!

cperkinsintel commented Jul 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raaiq1 Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raaiq1 Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raaiq1 commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader commented Jul 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raaiq1 commented Jun 21, 2022 •

edited

Loading

raaiq1 Jul 21, 2022 •

edited

Loading

raaiq1 Jul 21, 2022 •

edited

Loading

raaiq1 commented Jul 22, 2022 •

edited

Loading