[SYCL] Fix device code instrumentation (intel#4615)

Artem Gindinson · web-flow · commit 5667abaee1fd · 2021-09-29T21:41:13.000+03:00
- Fix the malfunctioning driver option for passing `-fsycl-instrument-device-code` to CC1 for SPIR-V-based targets. - Add the ITT device libraries when the CLI option is enabled, excluding these from `-fno-sycl-device-lib` effects. - Disable ITT annotations for ESIMD code. Tests are in intel/llvm-test-suite#484. Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
@@ -4331,20 +4331,19 @@ class OffloadingActionBuilder final {
 
     bool addSYCLDeviceLibs(const ToolChain *TC, ActionList &DeviceLinkObjects,
                            bool isSpirvAOT, bool isMSVCEnv) {
-      enum SYCLDeviceLibType {
-        sycl_devicelib_wrapper,
-        sycl_devicelib_fallback
-      };
       struct DeviceLibOptInfo {
         StringRef devicelib_name;
         StringRef devicelib_option;
       };
 
       bool NoDeviceLibs = false;
       int NumOfDeviceLibLinked = 0;
-      // Currently, all SYCL device libraries will be linked by default
-      llvm::StringMap<bool> devicelib_link_info = {
-          {"libc", true}, {"libm-fp32", true}, {"libm-fp64", true}};
+      // Currently, all SYCL device libraries will be linked by default. Linkage
+      // of "internal" libraries cannot be affected via -fno-sycl-device-lib.
+      llvm::StringMap<bool> devicelib_link_info = {{"libc", true},
+                                                   {"libm-fp32", true},
+                                                   {"libm-fp64", true},
+                                                   {"internal", true}};
       if (Arg *A = Args.getLastArg(options::OPT_fsycl_device_lib_EQ,
                                    options::OPT_fno_sycl_device_lib_EQ)) {
         if (A->getValues().size() == 0)
@@ -4357,11 +4356,16 @@ class OffloadingActionBuilder final {
           for (StringRef Val : A->getValues()) {
             if (Val == "all") {
               for (const auto &K : devicelib_link_info.keys())
-                devicelib_link_info[K] = true && !NoDeviceLibs;
+                devicelib_link_info[K] =
+                    true && (!NoDeviceLibs || K.equals("internal"));
               break;
             }
             auto LinkInfoIter = devicelib_link_info.find(Val);
-            if (LinkInfoIter == devicelib_link_info.end()) {
+            if (LinkInfoIter == devicelib_link_info.end() ||
+                Val.equals("internal")) {
+              // TODO: Move the diagnostic to the SYCL section of
+              // Driver::CreateOffloadingDeviceToolChains() to minimize code
+              // duplication.
               C.getDriver().Diag(diag::err_drv_unsupported_option_argument)
                   << A->getOption().getName() << Val;
             }
@@ -4375,30 +4379,34 @@ class OffloadingActionBuilder final {
       SmallVector<SmallString<128>, 4> LibLocCandidates;
       SYCLTC->SYCLInstallation.getSYCLDeviceLibPath(LibLocCandidates);
       StringRef LibSuffix = isMSVCEnv ? ".obj" : ".o";
-      SmallVector<DeviceLibOptInfo, 5> sycl_device_wrapper_libs = {
+      using SYCLDeviceLibsList = SmallVector<DeviceLibOptInfo, 5>;
+      const SYCLDeviceLibsList sycl_device_wrapper_libs = {
           {"libsycl-crt", "libc"},
           {"libsycl-complex", "libm-fp32"},
           {"libsycl-complex-fp64", "libm-fp64"},
           {"libsycl-cmath", "libm-fp32"},
           {"libsycl-cmath-fp64", "libm-fp64"}};
       // For AOT compilation, we need to link sycl_device_fallback_libs as
       // default too.
-      SmallVector<DeviceLibOptInfo, 5> sycl_device_fallback_libs = {
+      const SYCLDeviceLibsList sycl_device_fallback_libs = {
           {"libsycl-fallback-cassert", "libc"},
           {"libsycl-fallback-cstring", "libc"},
           {"libsycl-fallback-complex", "libm-fp32"},
           {"libsycl-fallback-complex-fp64", "libm-fp64"},
           {"libsycl-fallback-cmath", "libm-fp32"},
           {"libsycl-fallback-cmath-fp64", "libm-fp64"}};
-      auto addInputs = [&](SYCLDeviceLibType t) {
-        auto sycl_libs = (t == sycl_devicelib_wrapper)
-                             ? sycl_device_wrapper_libs
-                             : sycl_device_fallback_libs;
+      // ITT annotation libraries are linked in separately whenever the device
+      // code instrumentation is enabled.
+      const SYCLDeviceLibsList sycl_device_annotation_libs = {
+          {"libsycl-itt-user-wrappers", "internal"},
+          {"libsycl-itt-compiler-wrappers", "internal"},
+          {"libsycl-itt-stubs", "internal"}};
+      auto addInputs = [&](const SYCLDeviceLibsList &LibsList) {
         bool LibLocSelected = false;
         for (const auto &LLCandidate : LibLocCandidates) {
           if (LibLocSelected)
             break;
-          for (const DeviceLibOptInfo &Lib : sycl_libs) {
+          for (const DeviceLibOptInfo &Lib : LibsList) {
             if (!devicelib_link_info[Lib.devicelib_option])
               continue;
             SmallString<128> LibName(LLCandidate);
@@ -4421,9 +4429,11 @@ class OffloadingActionBuilder final {
           }
         }
       };
-      addInputs(sycl_devicelib_wrapper);
+      addInputs(sycl_device_wrapper_libs);
       if (isSpirvAOT)
-        addInputs(sycl_devicelib_fallback);
+        addInputs(sycl_device_fallback_libs);
+      if (Args.hasArg(options::OPT_fsycl_instrument_device_code))
+        addInputs(sycl_device_annotation_libs);
       return NumOfDeviceLibLinked != 0;
     }
 
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4660,6 +4660,15 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
       CmdArgs.push_back("-fsycl-allow-func-ptr");
     }
 
+    // Forward -fsycl-instrument-device-code option to cc1. This option will
+    // only be used for SPIR-V-based targets.
+    if (Arg *A =
+            Args.getLastArgNoClaim(options::OPT_fsycl_instrument_device_code))
+      if (Triple.isSPIR()) {
+        A->claim();
+        CmdArgs.push_back("-fsycl-instrument-device-code");
+      }
+
     if (!SYCLStdArg) {
       // The user had not pass SYCL version, thus we'll employ no-sycl-strict
       // to allow address-space unqualified pointers in function params/return
@@ -6409,15 +6418,6 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   // Forward -cl options to -cc1
   RenderOpenCLOptions(Args, CmdArgs, InputType);
 
-  // Forward -fsycl-instrument-device-code option to cc1. This option can only
-  // be used with spir triple.
-  if (Arg *A = Args.getLastArg(options::OPT_fsycl_instrument_device_code)) {
-    if (!Triple.isSPIR())
-      D.Diag(diag::err_drv_unsupported_opt_for_target)
-          << A->getAsString(Args) << TripleStr;
-    CmdArgs.push_back("-fsycl-instrument-device-code");
-  }
-
   if (IsHIP) {
     if (Args.hasFlag(options::OPT_fhip_new_launch_api,
                      options::OPT_fno_hip_new_launch_api, true))
diff --git a/clang/lib/Driver/ToolChains/SYCL.cpp b/clang/lib/Driver/ToolChains/SYCL.cpp
@@ -146,12 +146,15 @@ void SYCL::constructLLVMForeachCommand(Compilation &C, const JobAction &JA,
 // The list should match pre-built SYCL device library files located in
 // compiler package. Once we add or remove any SYCL device library files,
 // the list should be updated accordingly.
-static llvm::SmallVector<StringRef, 10> SYCLDeviceLibList{
+static llvm::SmallVector<StringRef, 16> SYCLDeviceLibList{
     "crt",
     "cmath",
     "cmath-fp64",
     "complex",
     "complex-fp64",
+    "itt-compiler-wrappers",
+    "itt-stubs",
+    "itt-user-wrappers",
     "fallback-cassert",
     "fallback-cstring",
     "fallback-cmath",
diff --git a/clang/test/Driver/sycl-device-lib-win.cpp b/clang/test/Driver/sycl-device-lib-win.cpp
@@ -94,11 +94,16 @@
 
 /// test invalid value for -f[no-]sycl-device-lib
 // RUN: %clangxx -fsycl %s -fsycl-device-lib=libc,dummy -### 2>&1 \
-// RUN:   | FileCheck %s -check-prefix=SYCL_DEVICE_LIB_INVALID_VALUE
+// RUN:   | FileCheck %s -check-prefix=SYCL_DEVICE_LIB_INVALID_VALUE -DVal=dummy
 // RUN: %clangxx -fsycl %s -fno-sycl-device-lib=dummy,libm-fp32 -### 2>&1 \
-// RUN:   | FileCheck %s -check-prefix=SYCL_NO_DEVICE_LIB_INVALID_VALUE
-// SYCL_DEVICE_LIB_INVALID_VALUE: error: unsupported argument 'dummy' to option 'fsycl-device-lib='
-// SYCL_NO_DEVICE_LIB_INVALID_VALUE: error: unsupported argument 'dummy' to option 'fno-sycl-device-lib='
+// RUN:   | FileCheck %s -check-prefix=SYCL_NO_DEVICE_LIB_INVALID_VALUE -DVal=dummy
+// Do separate checks for the compiler-reserved "internal" value
+// RUN: %clangxx -fsycl %s -fsycl-device-lib=internal -### 2>&1		\
+// RUN:   | FileCheck %s -check-prefix=SYCL_DEVICE_LIB_INVALID_VALUE -DVal=internal
+// RUN: %clangxx -fsycl %s -fno-sycl-device-lib=internal -### 2>&1		\
+// RUN:   | FileCheck %s -check-prefix=SYCL_NO_DEVICE_LIB_INVALID_VALUE -DVal=internal
+// SYCL_DEVICE_LIB_INVALID_VALUE: error: unsupported argument '[[Val]]' to option 'fsycl-device-lib='
+// SYCL_NO_DEVICE_LIB_INVALID_VALUE: error: unsupported argument '[[Val]]' to option 'fno-sycl-device-lib='
 
 /// ###########################################################################
 /// test llvm-link behavior for linking device libraries
diff --git a/clang/test/Driver/sycl-device-lib.cpp b/clang/test/Driver/sycl-device-lib.cpp
@@ -95,11 +95,16 @@
 
 /// test invalid value for -f[no-]sycl-device-lib
 // RUN: %clangxx -fsycl %s -fsycl-device-lib=libc,dummy -### 2>&1 \
-// RUN:   | FileCheck %s -check-prefix=SYCL_DEVICE_LIB_INVALID_VALUE
+// RUN:   | FileCheck %s -check-prefix=SYCL_DEVICE_LIB_INVALID_VALUE -DVal=dummy
 // RUN: %clangxx -fsycl %s -fno-sycl-device-lib=dummy,libm-fp32 -### 2>&1 \
-// RUN:   | FileCheck %s -check-prefix=SYCL_NO_DEVICE_LIB_INVALID_VALUE
-// SYCL_DEVICE_LIB_INVALID_VALUE: error: unsupported argument 'dummy' to option 'fsycl-device-lib='
-// SYCL_NO_DEVICE_LIB_INVALID_VALUE: error: unsupported argument 'dummy' to option 'fno-sycl-device-lib='
+// RUN:   | FileCheck %s -check-prefix=SYCL_NO_DEVICE_LIB_INVALID_VALUE -DVal=dummy
+// Do separate checks for the compiler-reserved "internal" value
+// RUN: %clangxx -fsycl %s -fsycl-device-lib=internal -### 2>&1		\
+// RUN:   | FileCheck %s -check-prefix=SYCL_DEVICE_LIB_INVALID_VALUE -DVal=internal
+// RUN: %clangxx -fsycl %s -fno-sycl-device-lib=internal -### 2>&1		\
+// RUN:   | FileCheck %s -check-prefix=SYCL_NO_DEVICE_LIB_INVALID_VALUE -DVal=internal
+// SYCL_DEVICE_LIB_INVALID_VALUE: error: unsupported argument '[[Val]]' to option 'fsycl-device-lib='
+// SYCL_NO_DEVICE_LIB_INVALID_VALUE: error: unsupported argument '[[Val]]' to option 'fno-sycl-device-lib='
 
 /// ###########################################################################
 /// test llvm-link behavior for linking device libraries
diff --git a/clang/test/Driver/sycl-instrumentation.c b/clang/test/Driver/sycl-instrumentation.c
@@ -1,14 +1,26 @@
-/// Check that SPIR ITT instrumentation is disabled by default:
-// RUN: %clang -### %s 2>&1 \
-// RUN: | FileCheck -check-prefix=CHECK-DEFAULT %s
-// CHECK-DEFAULT-NOT: "-fsycl-instrument-device-code"
+/// Check if -fsycl-instrument-device-code is passed to device-side -cc1
+/// and if ITT device libraries are pulled in.
+/// The following conditions must be fulfilled:
+/// 1. A SPIR-V-based environment must be targetted
+/// 2. The corresponding Driver option must be enabled explicitly
 
-/// Check if "fsycl_instrument_device_code" is passed to -cc1:
-// RUN: %clang -### -fsycl-instrument-device-code %s 2>&1 \
-// RUN: | FileCheck -check-prefix=CHECK-ENABLED %s
-// CHECK-ENABLED: "-cc1"{{.*}} "-fsycl-instrument-device-code"
+// RUN: %clangxx -fsycl -fsycl-instrument-device-code -fsycl-targets=spir64 -### %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=CHECK-SPIRV,CHECK-HOST %s
+/// -fno-sycl-device-lib mustn't affect the linkage of ITT libraries
+// RUN: %clangxx -fsycl -fsycl-instrument-device-code -fno-sycl-device-lib=all -fsycl-targets=spir64 -### %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=CHECK-SPIRV %s
 
-/// Check if "fsycl_instrument_device_code" usage with a non-spirv target
-/// results in an error.
-// RUN: %clang -### -fsycl-instrument-device-code --target=x86 %s 2>&1
-// expected-error{{unsupported option '-fsycl-instrument-device-code' for target 'x86_64-unknown-linux-gnu'}}
+// CHECK-SPIRV: "-cc1"{{.*}} "-fsycl-is-device"{{.*}} "-fsycl-instrument-device-code"
+// CHECK-SPIRV: clang-offload-bundler{{.*}} "-type=o" "-targets=sycl-spir64-unknown-unknown" "-inputs={{.*}}libsycl-itt-user-wrappers.{{o|obj}}" "-outputs={{.*}}libsycl-itt-user-wrappers-{{.*}}.{{o|obj}}" "-unbundle"
+// CHECK-SPIRV: clang-offload-bundler{{.*}} "-type=o" "-targets=sycl-spir64-unknown-unknown" "-inputs={{.*}}libsycl-itt-compiler-wrappers.{{o|obj}}" "-outputs={{.*}}libsycl-itt-compiler-wrappers-{{.*}}.{{o|obj}}" "-unbundle"
+// CHECK-SPIRV: clang-offload-bundler{{.*}} "-type=o" "-targets=sycl-spir64-unknown-unknown" "-inputs={{.*}}libsycl-itt-stubs.{{o|obj}}" "-outputs={{.*}}libsycl-itt-stubs-{{.*}}.{{o|obj}}" "-unbundle"
+// CHECK-SPIRV: llvm-link{{.*}} "-only-needed" "{{.*}}" "-o" "{{.*}}.bc" "--suppress-warnings"
+// CHECK-HOST-NOT: "-cc1"{{.*}} "-fsycl-is-host"{{.*}} "-fsycl-instrument-device-code"
+
+// RUN: %clangxx -fsycl -fsycl-targets=spir64 -### %s 2>&1	\
+// RUN: | FileCheck -check-prefixes=CHECK-NONPASSED %s
+// RUN: %clangxx -fsycl -fsycl-targets=nvptx64-nvidia-cuda -fsycl-instrument-device-code -nocudalib -### %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=CHECK-WARNING,CHECK-NONPASSED %s
+// CHECK-NONPASSED-NOT: "-fsycl-instrument-device-code"
+// CHECK-NONPASSED-NOT: "-inputs={{.*}}libsycl-itt-{{.*}}.{{o|obj}}"
+// CHECK-WARNING: warning: argument unused during compilation: '-fsycl-instrument-device-code'
diff --git a/llvm/lib/Transforms/Instrumentation/SPIRITTAnnotations.cpp b/llvm/lib/Transforms/Instrumentation/SPIRITTAnnotations.cpp
@@ -244,7 +244,10 @@ PreservedAnalyses SPIRITTAnnotationsPass::run(Module &M,
       SPIRV_GROUP_FMAX,      SPIRV_GROUP_UMAX, SPIRV_GROUP_SMAX};
 
   for (Function &F : M) {
-    if (F.isDeclaration())
+    // Do not annotate:
+    // - declarations
+    // - ESIMD functions (TODO: consider enabling instrumentation)
+    if (F.isDeclaration() || F.getMetadata("sycl_explicit_simd"))
       continue;
 
     // Work item start/finish annotations are only for SPIR kernels
diff --git a/sycl/test/esimd/vadd.cpp b/sycl/test/esimd/vadd.cpp
@@ -4,6 +4,9 @@
 // Check that the code compiles with -O0 and -g
 // RUN: %clangxx -I %sycl_include %s -o %t.out -fsycl -O0
 // RUN: %clangxx -I %sycl_include %s -o %t.out -fsycl -O0 -g
+// Check that the code compiles with device code instrumentation enabled
+// RUN: %clangxx -I %sycl_include %s -o %t.out -fsycl \
+// RUN: -fsycl-instrument-device-code
 
 #include <CL/sycl.hpp>
 #include <sycl/ext/intel/experimental/esimd.hpp>
