Page MenuHomePhabricator

[PATCH] [clang] Create SPIRABIInfo to enable SPIR_FUNC calling convention
ClosedPublic

Authored by mibintc on Dec 5 2020, 7:27 AM.

Details

Summary

Background: Call to library arithmetic functions for div is emitted by the compiler and it set wrong “C” calling convention for calls to these functions, whereas library functions are declared with spir_function calling convention. Due to wrong calling convention, InstCombine optimization replaces such calls with “unreachable” instruction.

It looks like clang lacks SPIRABIInfo class which should specify default calling conventions for “system” function calls. SPIR supports only SPIR_FUNC and SPIR_KERNEL calling convention.

Diff Detail

Event Timeline

mibintc requested review of this revision.Dec 5 2020, 7:27 AM
mibintc created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2020, 7:27 AM
mibintc planned changes to this revision.Dec 7 2020, 5:16 AM

I'm going to make further changes to this patch, it's not working as desired.

bader added a subscriber: bader.Dec 7 2020, 8:21 AM
mibintc updated this revision to Diff 309932.Dec 7 2020, 8:44 AM

ready for review, thanks!

I'm going to make further changes to this patch, it's not working as desired.

that was a false alarm.

From my perspective I think this is right... I don't feel comfortable approving this however until someone from OpenCL/OpenMP takes a look.

There are many RuntimeCalls that are created in Clang, including _read_pipe, all the OpenMP runtime calls, etc. Shall they all have their calling conventions set from the ABIInfo? Currently those calls are being set to 0.

There are many RuntimeCalls that are created in Clang, including _read_pipe, all the OpenMP runtime calls, etc. Shall they all have their calling conventions set from the ABIInfo? Currently those calls are being set to 0.

Makes sense to apply this to all functions indeed, btw there is also getDefaultCallingConvention from SPIR that could be used...

mibintc updated this revision to Diff 310338.Dec 8 2020, 1:51 PM

Thanks @Anastasia ; I modified CGBuiltin.cpp to use EmitRuntimeCall when creating calls to runtime functions instead of Builder.CreateCall so this is setting the CallingConvention on the Call node using the ABIInfo. I changed a few lit tests that were failing because of this.

I have a question, the clang test CodeGenOpenCL/cl20-device-side-enqueue.cl has this function, device_side_enqueue_block_invoke_9 which is defined with spir_func CC. The call doesn't have that CC. Where is that call created? Seems like the call should have spir_func CC, I want to fix that.

Likewise device_side_enqueue_block_invoke_9_kernel has spir_kernel CC. How does the CC get set to spir_kernel, I don't see where that is happening

define internal spir_func void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* %.block_descriptor, i8 addrspace(3)* %p1, i8 addrspace(3)* %p2, i8 addrspace(3)* %p3) #1 {
define internal spir_kernel void @__device_side_enqueue_block_invoke_9_kernel(i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3) #5 {

call void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
mibintc updated this revision to Diff 310655.Dec 9 2020, 2:13 PM

I rebased and applied clang-format patch

mibintc updated this revision to Diff 311005.Dec 10 2020, 1:28 PM

Fixed a couple CallInst, setting the CallingConvention from information in the Function node. Updated LIT tests.

I'd like to push the patch this weekend, looking for review thanks!

Answering my own questions

Thanks @Anastasia ; I modified CGBuiltin.cpp to use EmitRuntimeCall when creating calls to runtime functions instead of Builder.CreateCall so this is setting the CallingConvention on the Call node using the ABIInfo. I changed a few lit tests that were failing because of this.

I have a question, the clang test CodeGenOpenCL/cl20-device-side-enqueue.cl has this function, device_side_enqueue_block_invoke_9 which is defined with spir_func CC. The call doesn't have that CC. Where is that call created? Seems like the call should have spir_func CC, I want to fix that.

I found the place where the call was created and updated the CC

Likewise device_side_enqueue_block_invoke_9_kernel has spir_kernel CC. How does the CC get set to spir_kernel, I don't see where that is happening

The function is declared with "OpenCL Kernel" calling convention, but target-specific ClangCallConvToLLVMCallConv converts that to SPIR_KERNEL that's why I couldn't find it when doing a text search in clang

define internal spir_func void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* %.block_descriptor, i8 addrspace(3)* %p1, i8 addrspace(3)* %p2, i8 addrspace(3)* %p3) #1 {
define internal spir_kernel void @__device_side_enqueue_block_invoke_9_kernel(i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3) #5 {

call void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
Anastasia accepted this revision.Dec 11 2020, 3:40 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Dec 11 2020, 3:40 AM
This revision was landed with ongoing or failed builds.Dec 12 2020, 5:48 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review!