This is an archive of the discontinued LLVM Phabricator instance.

Differential D98411

[OpenCL] Respect calling convention for builtin
ClosedPublic

Authored by ldrumm on Mar 11 2021, 4:00 AM.

Details

Reviewers
Anastasia
mibintc
bader
yaxunl
svenvh
Commits
rGfcfd3fda7190: [OpenCL] Respect calling convention for builtin
Summary

__translate_sampler_initializer has a calling convention of
spir_func, but clang generated calls to it using the default CC.

Instruction Combining was lowering these mismatching calling conventions
to store i1* undef which itself was subsequently lowered to a trap
instruction by simplifyCFG resulting in runtime SIGILL

There are arguably two bugs here: but whether there's any wisdom in
converting an obviously invalid call into a runtime crash over aborting
with a sensible error message will require further discussion. So for
now it's enough to set the right calling convention on the runtime
helper.

Diff Detail

Event Timeline

ldrumm created this revision.Mar 11 2021, 4:00 AM
Herald added a subscriber: wenlei. · View Herald TranscriptMar 11 2021, 4:00 AM
ldrumm requested review of this revision.Mar 11 2021, 4:00 AM
bader accepted this revision.EditedMar 11 2021, 4:14 AM

Thanks! LGTM.
I think we should consider deprecating spir_func calling convention in favor of default calling convention. I don't see much value in having it today.

This revision is now accepted and ready to land.Mar 11 2021, 4:14 AM
Harbormaster completed remote builds in B93271: Diff 329910.Mar 11 2021, 10:14 AM
ldrumm added a subscriber: svenvh.Mar 12 2021, 3:38 AM

@Anastasia @svenvh is this already covered by https://reviews.llvm.org/D98039 ?

svenvh accepted this revision.Mar 12 2021, 5:41 AM
In D98411#2621736, @ldrumm wrote:

@Anastasia @svenvh is this already covered by https://reviews.llvm.org/D98039 ?

D98039 adds the calling convention for OpenCL language builtins during Sema, so it should not affect code generation of calls to __translate_sampler_initializer.

Your fix LGTM, thanks!

Closed by commit rGfcfd3fda7190: [OpenCL] Respect calling convention for builtin (authored by ldrumm). · Explain WhyMar 15 2021, 10:27 AM
This revision was automatically updated to reflect the committed changes.
ldrumm added a commit: rGfcfd3fda7190: [OpenCL] Respect calling convention for builtin.
ldrumm added a comment.Mar 15 2021, 10:32 AM

Given this fixes a runtime crash bug, should it be backported to the relevant release branch?

Anastasia added a comment.Mar 16 2021, 4:34 AM
In D98411#2626613, @ldrumm wrote:

Given this fixes a runtime crash bug, should it be backported to the relevant release branch?

Sure, thanks!

ldrumm added a comment.Mar 16 2021, 4:49 AM
In D98411#2628592, @Anastasia wrote:
In D98411#2626613, @ldrumm wrote:

Given this fixes a runtime crash bug, should it be backported to the relevant release branch?

Sure, thanks!

https://bugs.llvm.org/show_bug.cgi?id=49605

Anastasia added inline comments.Mar 16 2021, 5:04 AM
clang/lib/CodeGen/CodeGenModule.cpp
6273

Btw I think we should be using CodeGenFunction::EmitRuntimeCall here instead because it sets the CC correctly through SPIRABIInfo.

ldrumm added inline comments.Mar 16 2021, 5:20 AM
clang/lib/CodeGen/CodeGenModule.cpp
6273

Thanks. I'll prepare a separate patch.

ldrumm marked an inline comment as not done.Mar 16 2021, 5:20 AM
ldrumm mentioned this in rG440f6bdf34f4: [OpenCL][NFCI] Prefer CodeGenFunction::EmitRuntimeCall.Mar 16 2021, 9:23 AM

Revision Contents

PathSize
clang/
lib/
CodeGen/
12 lines
test/
CodeGenOpenCL/
12 lines

Diff 329910

clang/lib/CodeGen/CodeGenModule.cpp

Show First 20 Lines • Show All 6,257 Lines • ▼ Show 20 Lines
} }
llvm::SanitizerStatReport &CodeGenModule::getSanStats() { llvm::SanitizerStatReport &CodeGenModule::getSanStats() {
if (!SanStats) if (!SanStats)
SanStats = std::make_unique<llvm::SanitizerStatReport>(&getModule()); SanStats = std::make_unique<llvm::SanitizerStatReport>(&getModule());
return *SanStats; return *SanStats;
} }
llvm::Value * llvm::Value *
CodeGenModule::createOpenCLIntToSamplerConversion(const Expr *E, CodeGenModule::createOpenCLIntToSamplerConversion(const Expr *E,
CodeGenFunction &CGF) { CodeGenFunction &CGF) {
llvm::Constant *C = ConstantEmitter(CGF).emitAbstract(E, E->getType()); llvm::Constant *C = ConstantEmitter(CGF).emitAbstract(E, E->getType());
auto SamplerT = getOpenCLRuntime().getSamplerType(E->getType().getTypePtr()); auto *SamplerT = getOpenCLRuntime().getSamplerType(E->getType().getTypePtr());
auto FTy = llvm::FunctionType::get(SamplerT, {C->getType()}, false); auto *FTy = llvm::FunctionType::get(SamplerT, {C->getType()}, false);
return CGF.Builder.CreateCall(CreateRuntimeFunction(FTy, auto *Call = CGF.Builder.CreateCall(
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

Btw I think we should be using CodeGenFunction::EmitRuntimeCall here instead because it sets the CC correctly through SPIRABIInfo.

Anastasia: Btw I think we should be using `CodeGenFunction::EmitRuntimeCall` here instead because it sets…
ldrummAuthorUnsubmitted
Not Done
ReplyInline Actions

Thanks. I'll prepare a separate patch.

ldrumm: Thanks. I'll prepare a separate patch.
"__translate_sampler_initializer"), CreateRuntimeFunction(FTy, "__translate_sampler_initializer"), {C});
{C}); Call->setCallingConv(Call->getCalledFunction()->getCallingConv());
return Call;
} }
CharUnits CodeGenModule::getNaturalPointeeTypeAlignment( CharUnits CodeGenModule::getNaturalPointeeTypeAlignment(
QualType T, LValueBaseInfo *BaseInfo, TBAAAccessInfo *TBAAInfo) { QualType T, LValueBaseInfo *BaseInfo, TBAAAccessInfo *TBAAInfo) {
return getNaturalTypeAlignment(T->getPointeeType(), BaseInfo, TBAAInfo, return getNaturalTypeAlignment(T->getPointeeType(), BaseInfo, TBAAInfo,
/* forPointeeType= */ true); /* forPointeeType= */ true);
} }
▲ Show 20 Lines • Show All 95 Lines • Show Last 20 Lines

clang/test/CodeGenOpenCL/sampler.cl

Show All 33 Lines
kernel void foo(sampler_t smp_par) { kernel void foo(sampler_t smp_par) {
// CHECK-LABEL: define{{.*}} spir_kernel void @foo(%opencl.sampler_t addrspace(2)* %smp_par) // CHECK-LABEL: define{{.*}} spir_kernel void @foo(%opencl.sampler_t addrspace(2)* %smp_par)
// CHECK: [[smp_par_ptr:%[A-Za-z0-9_\.]+]] = alloca %opencl.sampler_t addrspace(2)* // CHECK: [[smp_par_ptr:%[A-Za-z0-9_\.]+]] = alloca %opencl.sampler_t addrspace(2)*
// Case 2b // Case 2b
sampler_t smp = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_NEAREST; sampler_t smp = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_NEAREST;
// CHECK: [[smp_ptr:%[A-Za-z0-9_\.]+]] = alloca %opencl.sampler_t addrspace(2)* // CHECK: [[smp_ptr:%[A-Za-z0-9_\.]+]] = alloca %opencl.sampler_t addrspace(2)*
// CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 19) // CHECK: [[SAMP:%[0-9]+]] = call spir_func %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 19)
// CHECK: store %opencl.sampler_t addrspace(2)* [[SAMP]], %opencl.sampler_t addrspace(2)** [[smp_ptr]] // CHECK: store %opencl.sampler_t addrspace(2)* [[SAMP]], %opencl.sampler_t addrspace(2)** [[smp_ptr]]
// Case 1b // Case 1b
fnc4smp(smp); fnc4smp(smp);
// CHECK-NOT: call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 19) // CHECK-NOT: call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 19)
// CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[smp_ptr]] // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[smp_ptr]]
// CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]]) // CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]])
// Case 1b // Case 1b
fnc4smp(smp); fnc4smp(smp);
// CHECK-NOT: call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 19) // CHECK-NOT: call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 19)
// CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[smp_ptr]] // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[smp_ptr]]
// CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]]) // CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]])
// Case 1a/2a // Case 1a/2a
fnc4smp(glb_smp); fnc4smp(glb_smp);
// CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35) // CHECK: [[SAMP:%[0-9]+]] = call spir_func %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35)
// CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]]) // CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]])
// Case 1a/2c // Case 1a/2c
fnc4smp(glb_smp_const); fnc4smp(glb_smp_const);
// CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35) // CHECK: [[SAMP:%[0-9]+]] = call spir_func %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35)
// CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]]) // CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]])
// Case 1c // Case 1c
fnc4smp(smp_par); fnc4smp(smp_par);
// CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[smp_par_ptr]] // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[smp_par_ptr]]
// CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]]) // CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]])
fnc4smp(5); fnc4smp(5);
// CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 5) // CHECK: [[SAMP:%[0-9]+]] = call spir_func %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 5)
// CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]]) // CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]])
const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR; const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
fnc4smp(const_smp); fnc4smp(const_smp);
// CHECK: [[CONST_SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35) // CHECK: [[CONST_SAMP:%[0-9]+]] = call spir_func %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35)
// CHECK: store %opencl.sampler_t addrspace(2)* [[CONST_SAMP]], %opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR:%[a-zA-Z0-9]+]] // CHECK: store %opencl.sampler_t addrspace(2)* [[CONST_SAMP]], %opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR:%[a-zA-Z0-9]+]]
fnc4smp(const_smp); fnc4smp(const_smp);
// CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR]] // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR]]
// CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]]) // CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]])
constant sampler_t constant_smp = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR; constant sampler_t constant_smp = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
fnc4smp(constant_smp); fnc4smp(constant_smp);
// CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35) // CHECK: [[SAMP:%[0-9]+]] = call spir_func %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35)
// CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]]) // CHECK: call spir_func void [[FUNCNAME]](%opencl.sampler_t addrspace(2)* [[SAMP]])
// TODO: enable sampler initialization with non-constant integer. // TODO: enable sampler initialization with non-constant integer.
//const sampler_t const_smp_func_init = get_sampler_initializer(); //const sampler_t const_smp_func_init = get_sampler_initializer();
} }