[SPV-IR] Fix SPIRV builtin variable/call translation

[Nuullll]

This PR fixes the translation of SPIRV builtin variables/calls. The translator
now is able to recognize builtin-call-based SPV-IR input.
e.g. call spir_func i64 @_Z33__spirv_BuiltinGlobalInvocationIdi(i32 1)

• LLVM --> SPIRV translation:

  1. We first translate OCL work item builtins to SPV-IR builtin calls in
     OCLToSPIRV. For example:
     @_Z13get_global_idj(i) -> @_Z33__spirv_BuiltinGlobalInvocationIdi(i)

  2. SPV-IR builtin calls are transformed to variables in LLVMToSPIRV. This
     step is shared by all source inputs (SPV-IR, OCL-IR). For example:
     @_Z33__spirv_BuiltinGlobalInvocationIdi(i) ->
     x = load GlobalInvocationId; extract x, i

• SPIRV --> LLVM translation:

  1. We lower SPIRV builtin variables into calls in SPIRVToLLVM so that SPV-IR
     has builtin function calls (instead of variables) by default.

  2. Builtin calls in SPV-IR can be then translated into OpenCL builtin calls,
     in SPIRVToOCL.
     e.g. @_Z33__spirv_BuiltinGlobalInvocationIdi(i) -> @_Z13get_global_idj(i)

  3. SPIRVToOCL is also capable of lower SPIRV builtin variables into calls as
     preprocessing, so that this pass can be used by users independently to
     translate SPV-IR into OCL-IR.

Signed-off-by: Yilong Guo yilong.guo@intel.com

[Nuullll]

@AlexeySachkov @AlexeySotkin @Fznamznon @MrSidims Please take a look, thanks :)

[AlexeySotkin]

@Fznamznon could you please take a look?

[Fznamznon]

While we don't have an option to control output format, do we need another call to lowerBuiltinVariablesToCalls from SPIRToOCL passes?

[Nuullll]

Yes. I added for both SPIRVToOCL12 and SPIRVToOCL20.

lowerBuiltinVariablesToCalls is called at the start of SPIRVToOCL (as preprocessing), to make sure all work item builtins are used as calls, so that visitCallSPIRVBuiltin can convert them with a simple lookup.

[Fznamznon]

Yes, I understand that, thanks.
I see that lowerBuiltinVariablesToCalls is also called at the end of SPIRVReader, I guess it already makes SPIRVReader to produce calls to work item built-ins (instead of variables use). After that all that is left to do in SPIRVToOCL passes is to change built-in names in visitCallSPIRVBuiltin.
But lowerBuiltinVariablesToCalls is called again at the beginning of SPIRVToOCL pass. Do we really need this second call, or I just misunderstood something? (Sorry If I did).

[MrSidims]

I guess a comment on line 3328 "// TODO: add an option to control the builtin format in SPV-IR." explains why lowerBuiltinVariablesToCalls is being called twice and foresees a time when it actually won't be a case . BTW, why do we need to call lowerBuiltinVariablesToCalls in the reader in the first place? I'd prefer to always have GV-like built-ins in SPV-IR, or I just don't have enough data?

[Fznamznon]

I guess a comment on line 3328 "// TODO: add an option to control the builtin format in SPV-IR." explains why lowerBuiltinVariablesToCalls is being called twice and foresees a time when it actually won't be a case .

Yes, as I already mentioned in my first comment, while we don't have an option, can we make only one call?

BTW, why do we need to call lowerBuiltinVariablesToCalls in the reader in the first place? I'd prefer to always have GV-like built-ins in SPV-IR

The thing is, we already have users that use calls, so we don't want to break their workflow.

[Nuullll]

Yes, as I already mentioned in my first comment, while we don't have an option, can we make only one call?

Sorry I didn't make it clear. The thing is, we have users that use the SPIRVToOCL pass independently, translating SPV-IR into OCL-IR. And for these users, the SPIRVToOCL pass may receive SPV-IR of both formats (builtin calls / variables) as input. For example, clang is still emitting builtin-variable-based SPV-IR.
So I think this call in SPIRVToOCL is probably unavoidable, at least for the current stage :(

[Fznamznon]

Ah, I understand. Thank you.

[AlexeySachkov]

Why I and not F? If I understand correctly, here we iterate over functions and not over instructions

[Nuullll]

Done. A leftover during copy/paste :P

[Nuullll]

A gentle ping.

[Fznamznon]

Don't we have variable's name in DemangledName already?
I assume DemangledName is not fully demangled, right?

[Nuullll]

Thanks, done! Eliminated BVKind as well!

[Fznamznon]

NIT

Suggested change

	*M, GVType, true, GlobalValue::ExternalLinkage, nullptr, BuiltinVarName,
	*M, GVType, /*isConstant=*/ true, GlobalValue::ExternalLinkage, nullptr, BuiltinVarName,

It would be great to add comments explaining what constant values mean https://llvm.org/docs/CodingStandards.html#comment-formatting .

[Nuullll]

Done.

[Fznamznon]

NIT

Suggested change

	for (auto UI = F.user_begin(), UE = F.user_end(); UI != UE; ++UI) {
	for (auto *U : F->users()) {

[Nuullll]

Thanks, done.

[Fznamznon]

NIT: that would be weird if something git into ToRemove and it is not a call or a function, right?

Suggested change

	else if (auto *F = dyn_cast<Function>(V))
	F->eraseFromParent();
	else if (auto *F = dyn_cast<Function>(V))
	F->eraseFromParent();
	else
	llvm_unreachable("Unexpected value to remove!");

[Nuullll]

Done.

[Fznamznon]

I guess it is expected that a function named oclIsBuiltin returns true for SPV-IR as well? Maybe we should rename it?

[Nuullll]

Yes. I think we better do it (renaming oclIsBuiltin()) in a separate PR to avoid irrelevant changes.

[Fznamznon]

We usually call this representation SPV-IR, right?

Suggested change

	/// Transforms SPR-IR work-item builtin calls to SPIRV builtin variables.
	/// e.g.
	/// SPR-IR: @_Z33__spirv_BuiltInGlobalInvocationIdi(i)
	/// is transformed as:
	/// x = load GlobalInvocationId; extract x, i
	/// e.g.
	/// SPR-IR: @_Z22__spirv_BuiltInWorkDimi()
	/// is transformed as:
	/// load WorkDim
	/// Transforms SPV-IR work-item builtin calls to SPIRV builtin variables.
	/// e.g.
	/// SPV-IR: @_Z33__spirv_BuiltInGlobalInvocationIdi(i)
	/// is transformed as:
	/// x = load GlobalInvocationId; extract x, i
	/// e.g.
	/// SPV-IR: @_Z22__spirv_BuiltInWorkDim()
	/// is transformed as:
	/// load WorkDim

(also corrected mangling).

[Nuullll]

Typo fixed.

[Fznamznon]

Suggested change

	// Store instructions and functions need to be removed.
	// Store instructions and functions that need to be removed.

[Nuullll]

Thanks, done.

[Fznamznon]

Yes, I understand that, thanks.
I see that lowerBuiltinVariablesToCalls is also called at the end of SPIRVReader, I guess it already makes SPIRVReader to produce calls to work item built-ins (instead of variables use). After that all that is left to do in SPIRVToOCL passes is to change built-in names in visitCallSPIRVBuiltin.
But lowerBuiltinVariablesToCalls is called again at the beginning of SPIRVToOCL pass. Do we really need this second call, or I just misunderstood something? (Sorry If I did).

[Fznamznon]

Thank you!