Page MenuHomePhabricator

[OpenCL] Respect calling convention for builtin
ClosedPublic

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

Details

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
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
svenvh accepted this revision.Mar 12 2021, 5:41 AM

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!

This revision was automatically updated to reflect the committed changes.

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

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

Sure, thanks!

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