[SYCL] Add support for __regcall calling convention to spir targets. #4533

kbobrovs · 2021-09-09T21:10:15Z

This calling convention makes compiler return values and function arguments
passed as values (through virtual registers) in most cases. The implementation
is basically customizing SPIRABIInfo for CGFunctionInfo with the X86_RegCall
calling convention. Code generation is mostly borrowed from AMDGPUABIInfo, but
with some of the restrictions for passing by value removed.

Signed-off-by: kbobrovs Konstantin.S.Bobrovsky@intel.com

premanandrao

I don't have experience with the specifics of the change. Would request someone else more familiar with calling convention issues to do a more thorough review.

clang/test/CodeGenSYCL/regcall-cc-test.cpp

erichkeane · 2021-09-13T15:04:06Z

This generally seems ok, though I would probably suggest some additional testing for parameters.

Additionally, I don't think we'd want to accept this without a spec written somewhere (I'd vastly prefer in the clang tree somewhere) that we can compare the implementation to.

kbobrovs · 2021-09-14T00:19:50Z

This generally seems ok, though I would probably suggest some additional testing for parameters.

Additionally, I don't think we'd want to accept this without a spec written somewhere (I'd vastly prefer in the clang tree somewhere) that we can compare the implementation to.

Thanks! Could you please suggest the test cases? Also, what specific place within clang tree for the doc do you have in mind?

clang/test/CodeGenSYCL/regcall-cc-test.cpp

erichkeane · 2021-09-14T13:00:26Z

This generally seems ok, though I would probably suggest some additional testing for parameters.
Additionally, I don't think we'd want to accept this without a spec written somewhere (I'd vastly prefer in the clang tree somewhere) that we can compare the implementation to.

Thanks! Could you please suggest the test cases? Also, what specific place within clang tree for the doc do you have in mind?

/clang/docs is where documentation goes. There doesn't seem to be a separate file for it, but there is a similar ABI document for Clang Blocks.

As for tests:
-I'd like a few showing that we don't 'register out' like the rest of the ABIs.
-Cases where the same types end up getting used by both parameters and return values (to show we don't care, again, like in register cases).
-Tests that show alignment mismatches don't cause packing.
-Tests with more types. How are shorts/longs combined? How about chars and ints? etc.
-At least the documentation has to cover variadic arguments, I believe there is an effort to allow printf, so we'll likely need C variadics covered.

Though, your test names seem very confusing. It is odd that PassAsTwo32bitInts is passed as 8 i8s. But I expect such an ABI document to cover what you really mean.

kbobrovs · 2021-09-14T17:18:34Z

@erichkeane, ok will add more tests. What do you mean by "register out"?

erichkeane · 2021-09-14T17:28:46Z

@erichkeane, ok will add more tests. What do you mean by "register out"?

Typically with ABIs, we change how things are passed based on the number of 'registers' we have used so far. For example, actual reg-call will use 15 SSE registers and 8 Integer registers (IIRC). At a certain point we run out of registers, and have to pass on stack.

kbobrovs · 2021-09-15T00:25:41Z

@erichkeane, ok will add more tests. What do you mean by "register out"?

Typically with ABIs, we change how things are passed based on the number of 'registers' we have used so far. For example, actual reg-call will use 15 SSE registers and 8 Integer registers (IIRC). At a certain point we run out of registers, and have to pass on stack.

I see. I have a test for that then - AMDGPUABIInfo will register out at 16, I removed that for the SPIRVABIInfo and added a test with 24 parameters - StillPassThroughRegisters

AaronBallman · 2021-09-15T13:25:45Z

@erichkeane, ok will add more tests. What do you mean by "register out"?

Typically with ABIs, we change how things are passed based on the number of 'registers' we have used so far. For example, actual reg-call will use 15 SSE registers and 8 Integer registers (IIRC). At a certain point we run out of registers, and have to pass on stack.

I see. I have a test for that then - AMDGPUABIInfo will register out at 16, I removed that for the SPIRVABIInfo and added a test with 24 parameters - StillPassThroughRegisters

24 still sounds like a reasonable number of registers. I'm wondering how we behave with a more stressful stress test, like 500 parameters (or some other excessive number of registers that makes sense). Basically -- can we run out of registers with this calling convention in SPIR?

kbobrovs · 2022-03-09T08:19:58Z

@erichkeane, @AaronBallman - I'm resuming work on this after a big pause, sorry for delay. I addressed your comments - tests extended a lot (one has 130 ints to test for 'register out', various type combinations), also added a description of ABI. I added it to ESIMD, as I think we are not ready to officially support it for pure SYCL.

Please review.

This calling convention makes compiler return values and function arguments passed as values (through virtual registers) in most cases. The implementation is basically customizing SPIRABIInfo for CGFunctionInfo with the X86_RegCall calling convention. Code generation is mostly borrowed from AMDGPUABIInfo, but with some of the restrictions for passing by value removed. Signed-off-by: kbobrovs <Konstantin.S.Bobrovsky@intel.com>

AaronBallman

This is not my strong suit, but I didn't see anything that concerned me.

clang/lib/CodeGen/TargetInfo.cpp

sycl/doc/extensions/experimental/sycl_ext_intel_esimd/sycl_ext_intel_esimd.md

premanandrao · 2022-03-09T18:02:55Z

sycl/doc/extensions/experimental/sycl_ext_intel_esimd/sycl_ext_intel_esimd.md

+    * 5-8 bytes - array of 2 ints
+  Floating point types are not merged. Structure field alignment rules can
+  increase the calculated size compared to simple sum of `sizeof` of all the
+  types. If the total size exceeds 8, then:


Structure field alignment rules can increase the calculated size compared to simple sum of sizeof of all the types.

I think this can be removed since it is mentioned above when describing how size is calculated.

premanandrao · 2022-03-09T18:19:33Z

clang/test/CodeGenSYCL/regcall-cc-test.cpp

+// === TEST CASE: multi-level nested structs with one primitive type element at
+// the bottom, and one - at the top. The nested struct at the top is expected to
+// get "unwraped" by the compiler evaporating to the single element at the
+// bottom.


I think this comment is a copy-and-paste error describing the previous case.

oh, right, will fix

premanandrao · 2022-03-09T18:22:15Z

clang/test/CodeGenSYCL/regcall-cc-test.cpp

+// CHECK-DAG: %struct.CharCharShortFloat = type { i8, i8, i16, float }
+
+template SYCL_DEVICE CharCharShortFloat __regcall func<CharCharShortFloat>(CharCharShortFloat x);
+// CHECK-DAG: define weak_odr x86_regcallcc [2 x i32] @_Z16__regcall3__funcI18CharCharShortFloatET_S1_([2 x i32] %{{[0-9a-zA-Z_.]+}})


Could you please explain this? The doc says floating point types are not merged.

Thanks for catching! Will update the doc.

clang/test/CodeGenSYCL/regcall-cc-test.cpp

kbobrovs · 2022-03-10T06:31:18Z

I'll wait for @erichkeane's review to make review-related changes all at once to reduce stress on the CI system.

erichkeane · 2022-03-10T13:59:11Z

I have nothing to add, Aaron and Prem covered everything I believe.

elizabethandrews

Some minor nits.

elizabethandrews · 2022-03-09T20:08:13Z

clang/lib/Basic/Targets/SPIR.h

+      // Permit CC_X86RegCall which is used to mark external functions with
+      //   explicit simd or structure type arguments to pass them via registers.


Suggested change

// Permit CC_X86RegCall which is used to mark external functions with

// explicit simd or structure type arguments to pass them via registers.

// Permit CC_X86RegCall which is used to mark external functions with

// explicit simd or structure type arguments to pass them via registers.

elizabethandrews · 2022-03-09T20:08:56Z

clang/lib/Basic/Targets/SPIR.h

+    return (CC == CC_SpirFunction || CC == CC_OpenCLKernel ||
+            // Permit CC_X86RegCall which is used to mark external functions
+            // with
+            //   explicit simd or structure type arguments to pass them via
+            //   registers.


Suggested change

return (CC == CC_SpirFunction || CC == CC_OpenCLKernel ||

// Permit CC_X86RegCall which is used to mark external functions

// with

// explicit simd or structure type arguments to pass them via

// registers.

return (CC == CC_SpirFunction || CC == CC_OpenCLKernel ||

// Permit CC_X86RegCall which is used to mark external functions

// with explicit simd or structure type arguments to pass them via

// registers.

elizabethandrews · 2022-03-10T15:57:37Z

clang/test/CodeGenSYCL/regcall-cc-test.cpp

+
+// === TEST CASE: multi-level nested structs with one primitive type element at
+// the bottom, and one - at the top. The nested struct at the top is expected to
+// get "unwraped" by the compiler evaporating to the single element at the


Suggested change

// get "unwraped" by the compiler evaporating to the single element at the

// get "unwrapped" by the compiler evaporating to the single element at the

thanks for catching!

sycl/doc/extensions/experimental/sycl_ext_intel_esimd/sycl_ext_intel_esimd.md

Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

kbobrovs · 2022-03-12T05:50:34Z

@intel/dpcpp-esimd-reviewers, friendly ping

kbobrovs · 2022-03-13T19:00:18Z

SYCL / Linux / L0 GEN9 LLVM Test Suite (pull_request) failed due to timeouts on unrelated tests:

Timed Out Tests (7):
SYCL :: KernelAndProgram/multiple-kernel-linking.cpp
SYCL :: Plugin/level_zero_batch_test.cpp
SYCL :: Plugin/level_zero_batch_test_copy_with_compute.cpp
SYCL :: Plugin/level_zero_dynamic_batch_test.cpp
SYCL :: Plugin/retain_events.cpp
SYCL :: Printf/char.cpp
SYCL :: Printf/float.cpp

Testing Time: 1080.59s
Unsupported : 184
Passed : 578
Expectedly Failed: 38
Timed Out : 7

kbobrovs · 2022-03-13T19:02:22Z

@premanandrao, @elizabethandrews, @erichkeane, @AaronBallman - review comments addressed, please take a look/approve

AaronBallman

LGTM, though I spotted one more typo in the docs

sycl/doc/extensions/experimental/sycl_ext_intel_esimd/sycl_ext_intel_esimd.md

clang/lib/CodeGen/TargetInfo.cpp

premanandrao

LGTM, just a couple of typos in the doc.

sycl/doc/extensions/experimental/sycl_ext_intel_esimd/sycl_ext_intel_esimd.md

v-klochkov

There are several misprints mentioned by reviewers, should they be fixed before merging?

premanandrao · 2022-03-14T19:15:14Z

There are several misprints mentioned by reviewers, should they be fixed before merging?

That would be my preference.

clang/lib/CodeGen/TargetInfo.cpp

Co-authored-by: Aaron Ballman <aaron@aaronballman.com> Co-authored-by: premanandrao <premanand.m.rao@intel.com>

kbobrovs · 2022-03-14T22:53:35Z

misprints fixed

kbobrovs · 2022-03-17T16:24:26Z

There is a single failure below. It is unrelated to this change, internal ticket created. The test passes on Windows in all modes, on Linux with OpenCL BE, fails only on Linux with LevelZero BE.
[2022-03-15T05:57:52.148Z] Failed Tests (1):
[2022-03-15T05:57:52.148Z] SYCL :: Assert/assert_in_simultaneous_kernels.cpp

kbobrovs requested review from AaronBallman, elizabethandrews and premanandrao as code owners September 9, 2021 21:10

kbobrovs requested review from erichkeane and rolandschulz September 9, 2021 21:10

kbobrovs force-pushed the regcall branch from 235e1ef to 52fd8de Compare September 9, 2021 21:16

premanandrao reviewed Sep 10, 2021

View reviewed changes

clang/test/CodeGenSYCL/regcall-cc-test.cpp Outdated Show resolved Hide resolved

kbobrovs commented Sep 14, 2021

View reviewed changes

clang/test/CodeGenSYCL/regcall-cc-test.cpp Outdated Show resolved Hide resolved

kbobrovs force-pushed the regcall branch from 01d7e08 to 77a9ff7 Compare March 9, 2022 08:14

kbobrovs requested review from a team as code owners March 9, 2022 08:14

kbobrovs force-pushed the regcall branch from 77a9ff7 to f154d32 Compare March 9, 2022 08:22

AaronBallman previously approved these changes Mar 9, 2022

View reviewed changes

clang/lib/CodeGen/TargetInfo.cpp Show resolved Hide resolved

premanandrao reviewed Mar 9, 2022

View reviewed changes

elizabethandrews reviewed Mar 10, 2022

View reviewed changes

kbobrovs commented Mar 10, 2022

View reviewed changes

sycl/doc/extensions/experimental/sycl_ext_intel_esimd/sycl_ext_intel_esimd.md Outdated Show resolved Hide resolved

Address review comments

8e942de

Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

kbobrovs dismissed AaronBallman’s stale review via 8e942de March 12, 2022 05:49

kbobrovs requested a review from premanandrao March 12, 2022 05:49

kbobrovs requested a review from elizabethandrews March 12, 2022 05:49

smanna12 mentioned this pull request Mar 13, 2022

[SYCL] Add clang support for device_global #5597

Merged

AaronBallman previously approved these changes Mar 14, 2022

View reviewed changes

sycl/doc/extensions/experimental/sycl_ext_intel_esimd/sycl_ext_intel_esimd.md Outdated Show resolved Hide resolved

smanna12 reviewed Mar 14, 2022

View reviewed changes

clang/lib/CodeGen/TargetInfo.cpp Show resolved Hide resolved

smanna12 reviewed Mar 14, 2022

View reviewed changes

clang/lib/CodeGen/TargetInfo.cpp Show resolved Hide resolved

smanna12 reviewed Mar 14, 2022

View reviewed changes

clang/lib/CodeGen/TargetInfo.cpp Show resolved Hide resolved

smanna12 reviewed Mar 14, 2022

View reviewed changes

clang/lib/CodeGen/TargetInfo.cpp Outdated Show resolved Hide resolved

premanandrao previously approved these changes Mar 14, 2022

View reviewed changes

sycl/doc/extensions/experimental/sycl_ext_intel_esimd/sycl_ext_intel_esimd.md Outdated Show resolved Hide resolved

sycl/doc/extensions/experimental/sycl_ext_intel_esimd/sycl_ext_intel_esimd.md Outdated Show resolved Hide resolved

v-klochkov previously approved these changes Mar 14, 2022

View reviewed changes

kbobrovs commented Mar 14, 2022

View reviewed changes

clang/lib/CodeGen/TargetInfo.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

2568239

Co-authored-by: Aaron Ballman <aaron@aaronballman.com> Co-authored-by: premanandrao <premanand.m.rao@intel.com>

kbobrovs dismissed stale reviews from v-klochkov, premanandrao, and AaronBallman via 2568239 March 14, 2022 22:45

kbobrovs requested review from smanna12, v-klochkov, premanandrao and AaronBallman March 14, 2022 22:53

smanna12 approved these changes Mar 14, 2022

View reviewed changes

v-klochkov approved these changes Mar 14, 2022

View reviewed changes

premanandrao approved these changes Mar 15, 2022

View reviewed changes

kbobrovs merged commit 4072557 into intel:sycl Mar 17, 2022

kbobrovs deleted the regcall branch March 17, 2022 17:05

KseniyaTikhomirova mentioned this pull request Mar 23, 2022

[SYCL] Disable test assert_in_simultaneous_kernels due to flaky failures intel/llvm-test-suite#935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Add support for __regcall calling convention to spir targets. #4533

[SYCL] Add support for __regcall calling convention to spir targets. #4533

kbobrovs commented Sep 9, 2021

premanandrao left a comment

erichkeane commented Sep 13, 2021

kbobrovs commented Sep 14, 2021

erichkeane commented Sep 14, 2021

kbobrovs commented Sep 14, 2021

erichkeane commented Sep 14, 2021

kbobrovs commented Sep 15, 2021

AaronBallman commented Sep 15, 2021

kbobrovs commented Mar 9, 2022

AaronBallman left a comment

premanandrao Mar 9, 2022

kbobrovs Mar 9, 2022

premanandrao Mar 9, 2022

kbobrovs Mar 9, 2022

premanandrao Mar 9, 2022

kbobrovs Mar 9, 2022

kbobrovs commented Mar 10, 2022

erichkeane commented Mar 10, 2022

elizabethandrews left a comment

elizabethandrews Mar 9, 2022

kbobrovs Mar 10, 2022

elizabethandrews Mar 9, 2022

kbobrovs Mar 10, 2022

elizabethandrews Mar 10, 2022

kbobrovs Mar 10, 2022

kbobrovs commented Mar 12, 2022

kbobrovs commented Mar 13, 2022

kbobrovs commented Mar 13, 2022

AaronBallman left a comment

premanandrao left a comment

v-klochkov left a comment

premanandrao commented Mar 14, 2022

kbobrovs commented Mar 14, 2022

kbobrovs commented Mar 17, 2022

		// Permit CC_X86RegCall which is used to mark external functions with
		// explicit simd or structure type arguments to pass them via registers.

	// get "unwraped" by the compiler evaporating to the single element at the
	// get "unwrapped" by the compiler evaporating to the single element at the

[SYCL] Add support for __regcall calling convention to spir targets. #4533

[SYCL] Add support for __regcall calling convention to spir targets. #4533

Conversation

kbobrovs commented Sep 9, 2021

premanandrao left a comment

Choose a reason for hiding this comment

erichkeane commented Sep 13, 2021

kbobrovs commented Sep 14, 2021

erichkeane commented Sep 14, 2021

kbobrovs commented Sep 14, 2021

erichkeane commented Sep 14, 2021

kbobrovs commented Sep 15, 2021

AaronBallman commented Sep 15, 2021

kbobrovs commented Mar 9, 2022

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbobrovs commented Mar 10, 2022

erichkeane commented Mar 10, 2022

elizabethandrews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbobrovs commented Mar 12, 2022

kbobrovs commented Mar 13, 2022

kbobrovs commented Mar 13, 2022

AaronBallman left a comment

Choose a reason for hiding this comment

premanandrao left a comment

Choose a reason for hiding this comment

v-klochkov left a comment

Choose a reason for hiding this comment

premanandrao commented Mar 14, 2022

kbobrovs commented Mar 14, 2022

kbobrovs commented Mar 17, 2022