This is an archive of the discontinued LLVM Phabricator instance.

Differential D92721

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

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

Details

Reviewers
v_klochkov
Anastasia
yaxunl
erichkeane
aaron.ballman
Commits
rG320af6b13839: Create SPIRABIInfo to enable SPIR_FUNC calling convention.
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

Unit TestsFailed

TimeTest
270 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::alloca_constant_size.cpp
250 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::alloca_loop_unpoisoning.cpp
750 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::asan_update_allocation.cpp
290 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::debug_locate.cpp
280 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::debug_stacks.cpp
View Full Test Results (355 Failed)

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
Harbormaster completed remote builds in B81210: Diff 309738.Dec 5 2020, 8:25 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!

mibintc added a comment.Dec 7 2020, 8:46 AM
In D92721#2436917, @mibintc wrote:

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

that was a false alarm.

Harbormaster completed remote builds in B81305: Diff 309932.Dec 7 2020, 9:17 AM
mibintc added a reviewer: erichkeane.Dec 7 2020, 12:28 PM
erichkeane added a comment.Dec 7 2020, 12:42 PM

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

mibintc added a comment.Dec 7 2020, 12:47 PM

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.

Anastasia added a comment.Dec 8 2020, 7:37 AM
In D92721#2437941, @mibintc wrote:

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)
Harbormaster completed remote builds in B81521: Diff 310338.Dec 8 2020, 2:47 PM
mibintc updated this revision to Diff 310655.Dec 9 2020, 2:13 PM

I rebased and applied clang-format patch

Harbormaster completed remote builds in B81711: Diff 310655.Dec 9 2020, 4:52 PM
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!

mibintc added a comment.Dec 10 2020, 1:31 PM

Answering my own questions

In D92721#2441068, @mibintc wrote:

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)
mibintc added a reviewer: aaron.ballman.Dec 10 2020, 1:32 PM
Harbormaster completed remote builds in B81903: Diff 311005.Dec 10 2020, 3:00 PM
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
Closed by commit rG320af6b13839: Create SPIRABIInfo to enable SPIR_FUNC calling convention. (authored by mibintc). · Explain Why
This revision was automatically updated to reflect the committed changes.
mibintc added a commit: rG320af6b13839: Create SPIRABIInfo to enable SPIR_FUNC calling convention..
mibintc added a comment.Dec 12 2020, 9:58 AM

Thanks for the review!

Revision Contents

PathSize
clang/
lib/
CodeGen/
17 lines
test/
CodeGenOpenCLCXX/
6 lines

Diff 309738

clang/lib/CodeGen/TargetInfo.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 9,974 Lines • ▼ Show 20 Lines for (unsigned I = 0; I != MangledDeclNames.size(); ++I) {
} }
} }
} }
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// SPIR ABI Implementation // SPIR ABI Implementation
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
namespace { namespace {
class SPIRABIInfo : public DefaultABIInfo {
public:
SPIRABIInfo(CodeGenTypes &CGT) : DefaultABIInfo(CGT) {
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-  SPIRABIInfo(CodeGenTypes &CGT) : DefaultABIInfo(CGT) {
-    setCCs();
-  }
+  SPIRABIInfo(CodeGenTypes &CGT) : DefaultABIInfo(CGT) { setCCs(); }
Lint: Pre-merge checks: clang-format: please reformat the code ``` - SPIRABIInfo(CodeGenTypes &CGT) : DefaultABIInfo…
setCCs();
}
private:
void setCCs();
};
} // end anonymous namespace
namespace {
class SPIRTargetCodeGenInfo : public TargetCodeGenInfo { class SPIRTargetCodeGenInfo : public TargetCodeGenInfo {
public: public:
SPIRTargetCodeGenInfo(CodeGen::CodeGenTypes &CGT) SPIRTargetCodeGenInfo(CodeGen::CodeGenTypes &CGT)
: TargetCodeGenInfo(std::make_unique<DefaultABIInfo>(CGT)) {} : TargetCodeGenInfo(std::make_unique<SPIRABIInfo>(CGT)) {}
unsigned getOpenCLKernelCallingConv() const override; unsigned getOpenCLKernelCallingConv() const override;
}; };
} // End anonymous namespace. } // End anonymous namespace.
void SPIRABIInfo::setCCs() {
assert(getRuntimeCC() == llvm::CallingConv::C);
RuntimeCC = llvm::CallingConv::SPIR_FUNC;
}
namespace clang { namespace clang {
namespace CodeGen { namespace CodeGen {
void computeSPIRKernelABIInfo(CodeGenModule &CGM, CGFunctionInfo &FI) { void computeSPIRKernelABIInfo(CodeGenModule &CGM, CGFunctionInfo &FI) {
DefaultABIInfo SPIRABI(CGM.getTypes()); DefaultABIInfo SPIRABI(CGM.getTypes());
SPIRABI.computeInfo(FI); SPIRABI.computeInfo(FI);
} }
} }
▲ Show 20 Lines • Show All 1,175 Lines • Show Last 20 Lines

clang/test/CodeGenOpenCLCXX/atexit.cl

//RUN: %clang_cc1 %s -triple spir -cl-std=clc++ -emit-llvm -O0 -o - | FileCheck %s //RUN: %clang_cc1 %s -triple spir -cl-std=clc++ -emit-llvm -O0 -o - | FileCheck %s
struct S { struct S {
~S(){}; ~S(){};
}; };
S s; S s;
//CHECK-LABEL: define internal void @__cxx_global_var_init() //CHECK-LABEL: define internal spir_func void @__cxx_global_var_init()
//CHECK: call i32 @__cxa_atexit(void (i8*)* bitcast (void (%struct.S addrspace(4)*)* @_ZNU3AS41SD1Ev to void (i8*)*), i8* null, i8 addrspace(1)* @__dso_handle) //CHECK: call spir_func i32 @__cxa_atexit(void (i8*)* bitcast (void (%struct.S addrspace(4)*)* @_ZNU3AS41SD1Ev to void (i8*)*), i8* null, i8 addrspace(1)* @__dso_handle)
//CHECK: declare i32 @__cxa_atexit(void (i8*)*, i8*, i8 addrspace(1)*) //CHECK: declare spir_func i32 @__cxa_atexit(void (i8*)*, i8*, i8 addrspace(1)*)