[NFC] Optimize OCLUtil::toString usage in checkError function

[vmaksimo]

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.

[vmaksimo]

Tagging @AlexeySachkov @MrSidims, please take a look

[AlexeySotkin]

Suggested change

	toString(V) + "\nAtomic " +
	V + "\nAtomic " +

[vmaksimo]

Do you mean such change? It'll not work now as checkError accepts an Instruction, not Value.

Suggested change

	toString(V) + "\nAtomic " +
	V, "\nAtomic " +

[AlexeySotkin]

Should we change checkError to accept Value?

[vmaksimo]

Applied in ff6cabe.

[Fznamznon]

There is a little hotspot involving toString in LLVMToSPIRVBase::transIndirectCallInst function. In case module contains a lot of indirect call instructions but the corresponding extension is enabled it is not necessary to call toString each time.
Using your changes, I was able to win some time by changing this:

 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);

to this

 SPIRVValue *LLVMToSPIRVBase::transIndirectCallInst(CallInst *CI,
                                                    SPIRVBasicBlock *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;

You can also include this improvement to your patch. Otherwise I'll commit it separately.

[vmaksimo]

@Fznamznon thanks for the nice suggestion! I'll apply this change too.

[MrSidims]

LGTM

[AlexeySotkin]

It makes passing Value to checkError() mandatory. I'm not sure which way is better: this assert or return an empty string in this case. @vmaksimo what do you think?

[vmaksimo]

Well, now it seems like it'd be enough to return an empty string - toString is a utility function and I would not expect it to fail often. I'll rewrite this a bit.