[NFC] Optimize OCLUtil::toString usage in checkError function (Khrono…

…sGroup#1229)

`toString` function is called to convert an LLVM IR instruction to a string,
mostly in order to include it into an error message in case LLVM IR is invalid.
The problem is that this function is called even when IR is valid which
significantly slows translator's work.

This patch reworks `toString` usage in `checkError` method to generate a string
only in case when LLVM IR is invalid.

lib/SPIRV/OCLUtil.h

@@ -495,14 +495,6 @@ bool isSamplerTy(Type *Ty);
 // Checks if the binary operator is an unfused fmul + fadd instruction.
 bool isUnfusedMulAdd(BinaryOperator *B);
 
-template <typename T> std::string toString(const T *Object) {
-  std::string S;
-  llvm::raw_string_ostream RSOS(S);
-  Object->print(RSOS);
-  RSOS.flush();
-  return S;
-}
-
 // Get data and vector size postfix for sugroup_block_{read|write} builtins
 // as specified by cl_intel_subgroups* extensions.
 // Scalar data assumed to be represented as vector of one element.

lib/SPIRV/SPIRVWriter.cpp

@@ -1010,9 +1010,8 @@ SPIRV::SPIRVInstruction *LLVMToSPIRVBase::transUnaryInst(UnaryInstruction *U,
     const auto DestAddrSpace = Cast->getDestTy()->getPointerAddressSpace();
     if (DestAddrSpace == SPIRAS_Generic) {
       getErrorLog().checkError(
-          SrcAddrSpace != SPIRAS_Constant, SPIRVEC_InvalidModule,
-          "Casts from constant address space to generic are illegal\n" +
-              toString(U));
+          SrcAddrSpace != SPIRAS_Constant, SPIRVEC_InvalidModule, U,
+          "Casts from constant address space to generic are illegal\n");
       BOC = OpPtrCastToGeneric;
       // In SPIR-V only casts to/from generic are allowed. But with
       // SPV_INTEL_usm_storage_classes we can also have casts from global_device
@@ -1021,10 +1020,9 @@ SPIRV::SPIRVInstruction *LLVMToSPIRVBase::transUnaryInst(UnaryInstruction *U,
                SrcAddrSpace == SPIRAS_GlobalHost) {
       getErrorLog().checkError(DestAddrSpace == SPIRAS_Global ||
                                    DestAddrSpace == SPIRAS_Generic,
-                               SPIRVEC_InvalidModule,
+                               SPIRVEC_InvalidModule, U,
                                "Casts from global_device/global_host only "
-                               "allowed to global/generic\n" +
-                                   toString(U));
+                               "allowed to global/generic\n");
       if (!BM->isAllowedToUseExtension(
               ExtensionID::SPV_INTEL_usm_storage_classes)) {
         if (DestAddrSpace == SPIRAS_Global)
@@ -1037,10 +1035,9 @@ SPIRV::SPIRVInstruction *LLVMToSPIRVBase::transUnaryInst(UnaryInstruction *U,
                DestAddrSpace == SPIRAS_GlobalHost) {
       getErrorLog().checkError(SrcAddrSpace == SPIRAS_Global ||
                                    SrcAddrSpace == SPIRAS_Generic,
-                               SPIRVEC_InvalidModule,
+                               SPIRVEC_InvalidModule, U,
                                "Casts to global_device/global_host only "
-                               "allowed from global/generic\n" +
-                                   toString(U));
+                               "allowed from global/generic\n");
       if (!BM->isAllowedToUseExtension(
               ExtensionID::SPV_INTEL_usm_storage_classes)) {
         if (SrcAddrSpace == SPIRAS_Global)
@@ -1051,14 +1048,12 @@ SPIRV::SPIRVInstruction *LLVMToSPIRVBase::transUnaryInst(UnaryInstruction *U,
       }
     } else {
       getErrorLog().checkError(
-          SrcAddrSpace == SPIRAS_Generic, SPIRVEC_InvalidModule,
+          SrcAddrSpace == SPIRAS_Generic, SPIRVEC_InvalidModule, U,
           "Casts from private/local/global address space are allowed only to "
-          "generic\n" +
-              toString(U));
+          "generic\n");
       getErrorLog().checkError(
-          DestAddrSpace != SPIRAS_Constant, SPIRVEC_InvalidModule,
-          "Casts from generic address space to constant are illegal\n" +
-              toString(U));
+          DestAddrSpace != SPIRAS_Constant, SPIRVEC_InvalidModule, U,
+          "Casts from generic address space to constant are illegal\n");
       BOC = OpGenericCastToPtr;
     }
   } else {
@@ -1876,9 +1871,8 @@ LLVMToSPIRVBase::transValueWithoutDecoration(Value *V, SPIRVBasicBlock *BB,
     AtomicRMWInst::BinOp Op = ARMW->getOperation();
     if (!BM->getErrorLog().checkError(
             !AtomicRMWInst::isFPOperation(Op) && Op != AtomicRMWInst::Nand,
-            SPIRVEC_InvalidInstruction,
-            OCLUtil::toString(V) + "\nAtomic " +
-                AtomicRMWInst::getOperationName(Op).str() +
+            SPIRVEC_InvalidInstruction, V,
+            "Atomic " + AtomicRMWInst::getOperationName(Op).str() +
                 " is not supported in SPIR-V!\n"))
       return nullptr;
 
@@ -3181,10 +3175,10 @@ SPIRVValue *LLVMToSPIRVBase::transIntrinsicInst(IntrinsicInst *II,
       return BM->addInstTemplate(OpSaveMemoryINTEL, BB, Ty);
     }
     BM->getErrorLog().checkError(
-        BM->isUnknownIntrinsicAllowed(II), SPIRVEC_InvalidFunctionCall,
-        toString(II) + "\nTranslation of llvm.stacksave intrinsic requires "
-                       "SPV_INTEL_variable_length_array extension or "
-                       "-spirv-allow-unknown-intrinsics option.");
+        BM->isUnknownIntrinsicAllowed(II), SPIRVEC_InvalidFunctionCall, II,
+        "Translation of llvm.stacksave intrinsic requires "
+        "SPV_INTEL_variable_length_array extension or "
+        "-spirv-allow-unknown-intrinsics option.");
     break;
   }
   case Intrinsic::stackrestore: {
@@ -3195,10 +3189,10 @@ SPIRVValue *LLVMToSPIRVBase::transIntrinsicInst(IntrinsicInst *II,
                                  nullptr);
     }
     BM->getErrorLog().checkError(
-        BM->isUnknownIntrinsicAllowed(II), SPIRVEC_InvalidFunctionCall,
-        toString(II) + "\nTranslation of llvm.restore intrinsic requires "
-                       "SPV_INTEL_variable_length_array extension or "
-                       "-spirv-allow-unknown-intrinsics option.");
+        BM->isUnknownIntrinsicAllowed(II), SPIRVEC_InvalidFunctionCall, II,
+        "Translation of llvm.restore intrinsic requires "
+        "SPV_INTEL_variable_length_array extension or "
+        "-spirv-allow-unknown-intrinsics option.");
     break;
   }
   // We can just ignore/drop some intrinsics, like optimizations hint.
@@ -3323,14 +3317,15 @@ SPIRVValue *LLVMToSPIRVBase::transDirectCallInst(CallInst *CI,
 
 SPIRVValue *LLVMToSPIRVBase::transIndirectCallInst(CallInst *CI,
                                                    SPIRVBasicBlock *BB) {
-  if (!BM->checkExtension(ExtensionID::SPV_INTEL_function_pointers,
-                          SPIRVEC_FunctionPointers, toString(CI)))
-    return nullptr;
-
-  return BM->addIndirectCallInst(
-      transValue(CI->getCalledOperand(), BB), transType(CI->getType()),
-      transArguments(CI, BB, SPIRVEntry::createUnique(OpFunctionCall).get()),
-      BB);
+  if (BM->getErrorLog().checkError(
+          BM->isAllowedToUseExtension(ExtensionID::SPV_INTEL_function_pointers),
+          SPIRVEC_FunctionPointers, CI)) {
+    return BM->addIndirectCallInst(
+        transValue(CI->getCalledOperand(), BB), transType(CI->getType()),
+        transArguments(CI, BB, SPIRVEntry::createUnique(OpFunctionCall).get()),
+        BB);
+  }
+  return nullptr;
 }
 
 SPIRVValue *LLVMToSPIRVBase::transAsmINTEL(InlineAsm *IA) {

lib/SPIRV/libSPIRV/SPIRVError.h

@@ -41,6 +41,7 @@
 
 #include "SPIRVDebug.h"
 #include "SPIRVUtil.h"
+#include "llvm/IR/Instruction.h"
 #include <iostream>
 #include <sstream>
 #include <string>
@@ -109,12 +110,35 @@ class SPIRVErrorLog {
                   const std::string &DetailedMsg = "",
                   const char *CondString = nullptr,
                   const char *FileName = nullptr, unsigned LineNumber = 0);
+  // Check if Condition is satisfied and set ErrCode and DetailedMsg with Value
+  // text representation if not. Returns true if no error.
+  bool checkError(bool Condition, SPIRVErrorCode ErrCode, llvm::Value *Value,
+                  const std::string &DetailedMsg = "",
+                  const char *CondString = nullptr,
+                  const char *FileName = nullptr, unsigned LineNumber = 0);
 
 protected:
   SPIRVErrorCode ErrorCode;
   std::string ErrorMsg;
 };
 
+inline bool SPIRVErrorLog::checkError(bool Cond, SPIRVErrorCode ErrCode,
+                                      llvm::Value *Value,
+                                      const std::string &Msg,
+                                      const char *CondString,
+                                      const char *FileName, unsigned LineNo) {
+  // Do early exit to avoid expensive toString() function call unless it is
+  // actually needed. That speeds up translator's execution.
+  if (Cond)
+    return Cond;
+  // Do not overwrite previous failure.
+  if (ErrorCode != SPIRVEC_Success)
+    return Cond;
+  std::string ValueIR = toString(Value);
+  return checkError(Cond, ErrCode, Msg + "\n" + ValueIR, CondString, FileName,
+                    LineNo);
+}
+
 inline bool SPIRVErrorLog::checkError(bool Cond, SPIRVErrorCode ErrCode,
                                       const std::string &Msg,
                                       const char *CondString,

lib/SPIRV/libSPIRV/SPIRVUtil.h

@@ -43,6 +43,8 @@
 #include <ostream>
 #define spv_ostream std::ostream
 
+#include "llvm/Support/raw_ostream.h"
+
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
@@ -422,6 +424,16 @@ getOrInsert(MapTy &Map, typename MapTy::key_type Key, FuncTy Func) {
   return NF;
 }
 
+template <typename T> std::string toString(const T *Object) {
+  if (Object == nullptr)
+    return "";
+  std::string S;
+  llvm::raw_string_ostream RSOS(S);
+  Object->print(RSOS);
+  RSOS.flush();
+  return S;
+}
+
 } // namespace SPIRV
 
 #endif // SPIRV_LIBSPIRV_SPIRVUTIL_H

test/negative/atomicrmw-unsupported-operation.ll

@@ -2,8 +2,8 @@
 ; RUN: not llvm-spirv %t.bc -o %t.spv 2>&1 | FileCheck %s
 
 ; CHECK: InvalidInstruction: Can't translate llvm instruction:
-; CHECK: atomicrmw nand i32 addrspace(1)* @ui, i32 42 acq_rel
 ; CHECK: Atomic nand is not supported in SPIR-V!
+; CHECK: atomicrmw nand i32 addrspace(1)* @ui, i32 42 acq_rel
 
 target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024"
 target triple = "spir64"

test/transcoding/SPV_INTEL_variable_length_array/negative.ll

@@ -3,8 +3,8 @@
 ; RUN: not llvm-spirv %t.bc -spirv-allow-unknown-intrinsics -o %t.spv 2>&1 | FileCheck %s --check-prefix=CHECK-ALLOCA
 
 ; CHECK-INTRINSIC: InvalidFunctionCall: Unexpected llvm intrinsic:
-; CHECK-INTRINSIC-NEXT: call i8* @llvm.stacksave()
 ; CHECK-INTRINSIC-NEXT: Translation of llvm.stacksave intrinsic requires SPV_INTEL_variable_length_array extension or -spirv-allow-unknown-intrinsics option.
+; CHECK-INTRINSIC-NEXT: call i8* @llvm.stacksave()
 
 ; CHECK-ALLOCA: InvalidInstruction: Can't translate llvm instruction:
 ; CHECK-ALLOCA-NEXT: %vla = alloca i32, i64 %a