[Clang] Improve generation of GV for virtual funcs #7449

KornevNikita · 2022-11-18T14:42:08Z

Reduce the code intended to generate valid device code global pointers with global address space in SYCL by introducing a new DefaultInt8PtrTy type.

KornevNikita · 2022-11-22T12:02:22Z

@intel/dpcpp-cfe-reviewers @intel/sycl-language-enabling-triage take a look please

clang/lib/CodeGen/ItaniumCXXABI.cpp

Naghasan · 2022-11-23T11:15:48Z

clang/test/CodeGenSYCL/no_opaque_virtual-types.cpp

@@ -20,9 +20,9 @@ int main() {
 // Struct layout big enough for vtable.
 // CHECK: %struct.Struct = type { i32 (...)** }
 // VTable:
-// CHECK: @_ZTV6Struct = linkonce_odr unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* bitcast ({ i8*, i8* }* @_ZTI6Struct to i8*), i8* bitcast (void (%struct.Struct addrspace(4)*)* @_ZN6Struct3fooEv to i8*)] }, comdat, align 8
-// CHECK: @[[TYPEINFO:.+]] = external global i8*
+// CHECK: @_ZTV6Struct = linkonce_odr unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* bitcast ({ i8 addrspace(4)*, i8* }* @_ZTI6Struct to i8*), i8* bitcast (void (%struct.Struct addrspace(4)*)* @_ZN6Struct3fooEv to i8*)] }, comdat, align 8


Sorry, I didn't paid attention to it before. That's going to create a GV the private address space (under the SPIR convention). The SPIR-V translator will map that to Function storage class, which you can't use outside a function. So AFAIK, the generated SPIR-V won't be valid.

Out of curiosity, is there an extension you are working with here ? That would help to understand if the test is correct.

Out of curiosity, is there an extension you are working with here ? That would help to understand if the test is correct.

We don't have any SYCL-level extensions and for SPIR-V level we have SPV_INTEL_function_pointers

sorry, I meant a SPIR-V extension.

Looking at the function pointer ext, wouldn't this expression
i8* bitcast (void (%struct.Struct addrspace(4)*)* @_ZN6Struct3fooEv to i8*) be a problem ? You can't cast to generic.

Originally it was a problem described in your first comment - we were receiving function storage class RTTI global variables and hence llvm-translator crash. Now it fixed because we receiving @_ZTVN10__cxxabiv117__class_type_infoE = external addrspace(1) global ptr addrspace(4), but not @_ZTVN10__cxxabiv117__class_type_infoE = external global ptr.
I believe it's not a problem because applications run successfully.

I believe it's not a problem because applications run successfully.

well, it is not a problem for a specific stack, which is quite different.

I can see the @[[TYPEINFO:.+]] was indeed change the way you described it, and I think it make sense. But my comment for _ZTV6Struct (and _ZTS6Struct / _ZTI6Struct) still stand: this will generate invalid SPIR-V.

i8* bitcast (void (%struct.Struct addrspace(4)*)* @_ZN6Struct3fooEv to i8*)

@Naghasan, here we don't cast from generic to private, we cast a private pointer to one type to private pointer to another type. We have a function pointer void(__generic Struct *) which we cast to i8* without changing the address space of that function pointer

You are right, I mixed up a few things in my brain by cross checking the test and the func ptr extension.

So in order to generate void (%struct.Struct addrspace(4)*)* @_ZN6Struct3fooEv you need to use OpConstantFunctionPointerINTEL which yield a pointer to storage class CodeSectionINTEL which you bitcast into a pointer to storage class Function. This is more what bugged me (I saw this by thinking of the implication of banning cast from generic storage class hence the confusion).

Strictly speaking, SPIR-V doesn't ban casting between storage class (at least I couldn't find wording banning this), which I found very strange (so does others, I'll seek clarification on that point).

So in order to generate void (%struct.Struct addrspace(4)) @_ZN6Struct3fooEv you need to use OpConstantFunctionPointerINTEL which yield a pointer to storage class CodeSectionINTEL which you bitcast into a pointer to storage class Function. This is more what bugged me (I saw this by thinking of the implication of banning cast from generic storage class hence the confusion).

In reality, we haven't implemented CodeSectionINTEL storage class or at least we haven't allocated a dedicated LLVM IR address space for it. For now it all just works with address spaces which are assigned by default to function pointers

we haven't implemented CodeSectionINTEL storage class

easy way to not hit the issue :)

clang/lib/CodeGen/CodeGenModule.cpp

premanandrao

I am okay with the changes.

Naghasan

Sorry, I hope you weren't waiting for my approval.

I still have the concern for the 2 points I highlighted but I think they should be addressed in different patches anyway.

elizabethandrews

Looks ok to me but please update the commit message to describe the change and why it was required.

elizabethandrews · 2022-11-30T11:51:06Z

clang/lib/CodeGen/ItaniumCXXABI.cpp

-    }
-  }
+  if (!VTable)
+    VTable = CGM.CreateRuntimeVariable(CGM.DefaultInt8PtrTy, VTableName);


So instead of global address space you will now create pointer in generic address space?

Yes, but the main idea is still preserved - the global variable pointer should be in the global address space:
... = external addrspace(1) global ptr addrspace(4)
And it's ok for us.
Are you good with this commit message?

I'm a little confused. Can you explain what this IR means. I know addresspace (1) = global and (4) is generic but what does it mean in here. What is in addresspace (4)?

By the way I don't want to hold up this patch. I'm asking all the questions so I understand something I am not very familiar with. Since you have approvals from other FE code owners, you can merge this

As I understand it, according to https://llvm.org/docs/LangRef.html#global-variables - here (in my example) addresspace (1) is the address space of the global variable, and (4) is the address space of the pointer it declares. @AlexeySachkov am I wrong?

Every global is a pointer in LLVM IR. When you have a pointer as a global variable, you essentially have a pointer to pointer. The top-level address space was incorrectly generated as 0 (private) and that is now fixed. With this patch with have a program-scope variable which is a pointer to generic address space, which in turns contains a pointer to a global address space (that pointer points to actual data somewhere).

bader · 2022-12-01T16:35:02Z

Shouldn't we commit this to llvm-project? Are there any dependencies on existing patches, except the test?

[Clang] Improve generation of GV for virtual funcs

1574817

KornevNikita requested a review from a team as a code owner November 18, 2022 14:42

KornevNikita requested a review from a team November 18, 2022 14:42

premanandrao reviewed Nov 22, 2022

View reviewed changes

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

Naghasan reviewed Nov 23, 2022

View reviewed changes

Rename variable

1ee9085

premanandrao approved these changes Nov 29, 2022

View reviewed changes

smanna12 approved these changes Nov 29, 2022

View reviewed changes

Naghasan approved these changes Nov 30, 2022

View reviewed changes

elizabethandrews approved these changes Nov 30, 2022

View reviewed changes

AlexeySachkov merged commit ca10db9 into intel:sycl Dec 1, 2022

KornevNikita deleted the improve-vtable-generation branch December 9, 2022 14:44

AlexeySachkov mentioned this pull request Aug 29, 2023

[SYCL-WEB] Sync VTable related customization with sycl branch #10993

Merged

[Clang] Improve generation of GV for virtual funcs #7449

[Clang] Improve generation of GV for virtual funcs #7449

Uh oh!

Conversation

KornevNikita commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KornevNikita commented Nov 22, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Naghasan left a comment

Choose a reason for hiding this comment

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader commented Dec 1, 2022

Uh oh!

Uh oh!

KornevNikita commented Nov 18, 2022 •

edited

Loading