[SYCL][NFC] Addressing some review comments

clang/include/clang/Driver/Driver.h

@@ -557,11 +557,11 @@ class Driver {
 
   /// Retrieves a ToolChain for a particular device \p Target triple
   ///
-  /// HostTC is the host ToolChain paired with the device
+  /// \param[in] HostTC is the host ToolChain paired with the device
   ///
-  /// Action (e.g. OFK_Cuda/OFK_OpenMP/OFK_SYCL) is an Offloading action that is
-  /// optionally passed to a ToolChain (used by CUDA, to specify if it's used in
-  /// conjunction with OpenMP)
+  /// \param[in] Action (e.g. OFK_Cuda/OFK_OpenMP/OFK_SYCL) is an Offloading
+  /// action that is optionally passed to a ToolChain (used by CUDA, to specify
+  /// if it's used in conjunction with OpenMP)
   ///
   /// Will cache ToolChains for the life of the driver object, and create them
   /// on-demand.

clang/lib/Driver/Driver.cpp

@@ -621,6 +621,9 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C,
         HostTriple.isArch64Bit() ? "nvptx64-nvidia-cuda" : "nvptx-nvidia-cuda";
     llvm::Triple CudaTriple(DeviceTripleStr);
 
+    // Use the device and host triple as the key into
+    // getOffloadingDeviceToolChain, because the device toolchain we create
+    // depends on both.
     auto CudaTC = &getOffloadingDeviceToolChain(C.getInputArgs(), CudaTriple,
                                                 *HostTC, OFK);
     C.addOffloadDeviceToolChain(CudaTC, OFK);
@@ -631,6 +634,9 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C,
     DeviceTripleStr = "amdgcn-amd-amdhsa";
     llvm::Triple HIPTriple(DeviceTripleStr);
 
+    // Use the device and host triple as the key into
+    // getOffloadingDeviceToolChain, because the device toolchain we create
+    // depends on both.
     auto HIPTC = &getOffloadingDeviceToolChain(C.getInputArgs(), HIPTriple,
                                               *HostTC, OFK);
     C.addOffloadDeviceToolChain(HIPTC, OFK);
@@ -767,6 +773,9 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C,
           else {
             const ToolChain *HostTC =
                 C.getSingleOffloadToolChain<Action::OFK_Host>();
+            // Use the device and host triple as the key into
+            // getOffloadingDeviceToolChain, because the device toolchain we
+            // create depends on both.
             auto SYCLTC = &getOffloadingDeviceToolChain(C.getInputArgs(), TT,
                                                      *HostTC, Action::OFK_SYCL);
             C.addOffloadDeviceToolChain(SYCLTC, Action::OFK_SYCL);
@@ -5109,34 +5118,36 @@ const ToolChain &Driver::getOffloadingDeviceToolChain(const ArgList &Args,
     // things.
     switch (TargetDeviceOffloadKind) {
       case Action::OFK_Cuda:
-          TC = llvm::make_unique<toolchains::CudaToolChain>(
-            *this, Target, HostTC, Args, TargetDeviceOffloadKind);
-      break;
+        TC = llvm::make_unique<toolchains::CudaToolChain>(
+          *this, Target, HostTC, Args, TargetDeviceOffloadKind);
+        break;
       case Action::OFK_HIP:
-          TC = llvm::make_unique<toolchains::HIPToolChain>(
-            *this, Target, HostTC, Args);
-      break;
+        TC = llvm::make_unique<toolchains::HIPToolChain>(
+          *this, Target, HostTC, Args);
+        break;
       case Action::OFK_OpenMP:
-          // omp + nvptx
-          TC = llvm::make_unique<toolchains::CudaToolChain>(
-            *this, Target, HostTC, Args, TargetDeviceOffloadKind);
-      break;
+        // omp + nvptx
+        TC = llvm::make_unique<toolchains::CudaToolChain>(
+          *this, Target, HostTC, Args, TargetDeviceOffloadKind);
+        break;
       case Action::OFK_SYCL:
         switch (Target.getArch()) {
           case llvm::Triple::spir:
           case llvm::Triple::spir64:
             TC = llvm::make_unique<toolchains::SYCLToolChain>(
               *this, Target, HostTC, Args);
-          break;
+            break;
           case llvm::Triple::fpga32:
           case llvm::Triple::fpga64:
             TC = llvm::make_unique<toolchains::XOCCToolChain>(
               *this, Target, HostTC, Args);
-          break;
+            break;
+          default:
+            llvm_unreachable("Unexpected option.");
         }
       break;
       default:
-      break;
+        llvm_unreachable("Unexpected option.");
     }
   }
 

clang/lib/Driver/ToolChains/Clang.cpp

@@ -3565,8 +3565,8 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   // triple of the thing currently being compiled. To do this we would need to
   // remove the reliance on a host side definition (__SYCL_XILINX_ONLY__)
   // inside of InitPreprocessor.cpp
-  if (IsSYCL &&
-      C.getSingleOffloadToolChain<Action::OFK_SYCL>()
+  if (IsSYCL
+      && C.getSingleOffloadToolChain<Action::OFK_SYCL>()
           ->getTriple().isXilinxFPGA())
     CmdArgs.push_back("-fsycl-xocc");
 

sycl/tools/sycl-xocc/bin/sycl-xocc

@@ -36,31 +36,48 @@
 # the environment variables are a quick workaround to swap boards. 
 usage() { echo xocc_compile: error: $2 >&2; exit $1; }
 
-[ $# -eq 0      ] && usage 1 "no sdx bin directory containing xocc"
-[ $# -eq 0      ] && usage 2 "no driver path specified"
-[ $# -eq 0      ] && usage 3 "no source file name"
-[ $# -eq 0      ] && usage 4 "no input file name"
-[ $# -eq 0      ] && usage 5 "no temporary directory specified"
-[ $# -eq 0      ] && usage 6 "no final output file specified"
+if [[ -z "$1" ]]; then
+  usage 1 "no sdx bin directory containing xocc"
+fi
+
+if [[ -z "$2" ]]; then
+  usage 2 "no driver path specified"
+fi
+
+if [[ -z "$3" ]]; then
+  usage 3 "no source file name"
+fi
+
+if [[ -z "$4" ]]; then
+  usage 4 "no input file name"
+fi
+
+if [[ -z "$5" ]]; then
+  usage 5 "no temporary directory specified"
+fi
+
+if [[ -z "$6" ]]; then
+  usage 6 "no final output file specified"
+fi
 
 # Hardcoded path from the bin directory that xocc is stored in, works for now 
 # but a future TODO might be to allow this to be optionally specified as an 
 # ENV variable 
-XOCC_LIB_SPIR=$1/../lnx64/lib/libspir64-39-hls.bc
+XOCC_LIB_SPIR="$1/../lnx64/lib/libspir64-39-hls.bc"
 
 # Perhaps not so important that the linker is the one packaged with the Clang
 # build, but it's important that Opt is so that it's packaged with the correct 
 # LLVM passes. 
-OPT=$2/opt
-LLVM_LINK=$2/llvm-link
-XOCC=$1/xocc
+OPT="$2/opt"
+LLVM_LINK="$2/llvm-link"
+XOCC="$1/xocc"
 
-$OPT -O3 -asfix -globaldce -inSPIRation -globaldce -kernelNameGen $4 -o \
-  $5/$3_kernels-optimized.bc
+$OPT -O3 -asfix -globaldce -inSPIRation -globaldce -kernelNameGen "$4" -o \
+  "$5/$3_kernels-optimized.bc"
 
 # Link our XOCC SPIR binaries in 
-$LLVM_LINK $5/$3_kernels-optimized.bc $XOCC_LIB_SPIR -o \
-  $5/$3_kernels-linked.xpirbc 
+$LLVM_LINK "$5/$3_kernels-optimized.bc" $XOCC_LIB_SPIR -o \
+  "$5/$3_kernels-linked.xpirbc" 
 
 # Our file containing the KernelNames is placed in the systems temporary 
 # directory (or wherever the Clang driver is told is the temporary directory) 
@@ -76,13 +93,13 @@ while IFS= read -r X
 do
 if [[ -n "$X" ]]; then
   $XOCC --target $XCL_EMULATION_MODE --platform $XILINX_PLATFORM \
-    --save-temps -c -k $X -o $5/$X.xo $5/$3_kernels-linked.xpirbc 
+    --save-temps -c -k "$X" -o "$5/$X.xo" "$5/$3_kernels-linked.xpirbc" 
     LINKER_LIST="$5/$X.xo $LINKER_LIST"
 fi
 done <<< $DATA
 
 $XOCC --target $XCL_EMULATION_MODE --platform $XILINX_PLATFORM -l \
-  --save-temps -o $6 $LINKER_LIST
+  --save-temps -o "$6" $LINKER_LIST
 
 # TODO: Cleanup temporary files we allocated in the systems tmp directory and 
 # when --save-temps is fixed for SYCL compilation optionally keep them as they