[SPV->LLVM] Fix global c/dtors type if SPV is from opaque type LLVM #2280

wenju-he · 2023-12-21T05:37:11Z

When SPV is generated from LLVM IR with opaque pointer enabled, ctor
function in global ctors has opaque pointer type rather than function
type. When translating the SPV back to LLVM IR with typed pointer like
in LLVM 14, ctor function type is casted to i8* pointer in the global
ctors initializer. This results in error in LLVM verifier.
This PR fixes the issue by removing the cast.

When SPV is generated from LLVM IR with opaque pointer enabled, ctor function in global ctors has opaque pointer type rather than function type. When translating the SPV back to LLVM IR with typed pointer like in LLVM 14, ctor function type is casted to i8* pointer in the global ctors initializer. This results in error in LLVM verifier. This PR fixes the issue by removing the cast.

wenju-he · 2023-12-21T07:03:05Z

The PR is needed for both llvm_release_140 and llvm_release_150 branches since opaque pointer isn't enabled by default until LLVM 16.

wenju-he · 2023-12-21T07:03:11Z

D:\a\SPIRV-LLVM-Translator\SPIRV-LLVM-Translator\llvm-project\llvm\projects\SPIRV-LLVM-Translator\lib\SPIRV\libSPIRV/SPIRVNameMapEnum.h(597,7): error C2065: 'CapabilityLongConstantCompositeINTEL': undeclared identifier (compiling source file D:\a\SPIRV-LLVM-Translator\SPIRV-LLVM-Translator\llvm-project\llvm\projects\SPIRV-LLVM-Translator\lib\SPIRV\OCLToSPIRV.cpp) [D:\a\SPIRV-LLVM-Translator\SPIRV-LLVM-Translator\build\projects\SPIRV-LLVM-Translator\lib\SPIRV\LLVMSPIRVLib.vcxproj]

Error in CI in-tree windows build isn't related to this PR

VyacheslavLevytskyy · 2023-12-21T13:01:29Z

lib/SPIRV/SPIRVReader.cpp

@@ -1331,6 +1331,39 @@ void SPIRVToLLVM::transFunctionPointerCallArgumentAttributes(
  }
 }

+/// When spirv is generated from LLVM IR with opapque pointer enabled and then


typo: opaque

VyacheslavLevytskyy · 2023-12-21T13:12:23Z

lib/SPIRV/SPIRVReader.cpp

+/// casted to i8* type in \p Ty and \p Initializer, which causes error in LLVM
+/// IR verifier. E.g.
+///   [1 x %0][%0 { i32 1, i8* bitcast (void ()* @ctor to i8*), i8* null }]
+/// This function removes the cast so that LLVM IR is valid.


I'd like to better understand the scope of the problem when we get an opaque pointer type rather than function
type, and so interpret it further like a i8* pointer when reverse translate it back to LLVM. Does this behavior manifest itself for ctor/dtor only, and that is caused by special treatment of ctor/dtor case in other places of the source code base? Or this is rather a wider problem that is not only about ctor/dtor?

I agree with your concern that this behavior could be a wider problem.
I was also thinking why this issue is exposed so late. My guess is that IGC who uses llvm-14 branch switched to use the open-source SPIRV-LLVM-Translator not long time ago, so some issues related to opaque types are not exposed yet.

ctor/dtor use function pointer. This problem here is kind of special that LLVM IR verifier specifically requires that the type of the second element should be a function type rather than i8* type, see https://github.com/llvm/llvm-project/blob/248fba0cd806a0f6bf4b0f12979f2185f2bed111/llvm/lib/IR/Verifier.cpp#L798

Indeed, the issue this PR addresses might also be related to cases that a function pointer is used in the code, e.g. as an indirect function call. With the second commit of this PR, I verified that SPV from opaque pointer in tests in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/tree/main/test/extensions/INTEL/SPV_INTEL_function_pointers could be translated to LLVM IR with typed pointer using SPIRV-LLVM-Translator llvm_release_140 branch. Therefore, function pointer handling seems good.

Other than function pointer, I don't know if opaque types handling in SPIRV-LLVM-Translator llvm_release_140 branch has more issues. As @LU-JOHN mentioned, we might need the whole SPIRVTypeScavenger.

VyacheslavLevytskyy · 2023-12-21T13:17:37Z

lib/SPIRV/SPIRVReader.cpp

+  unsigned NumEltsInArr = InitArr->getType()->getNumElements();
+  assert(NumEltsInArr && "array is empty");
+  auto *CS = cast<ConstantStruct>(InitArr->getAggregateElement(0u));
+  assert(CS->getType()->getNumElements() == 3 && "expect 3 elements in struct");


This new function is not added to validate, so I'm not sure you need new asserts here at all. It looks like the following logic can be just protected by if conditions, for example, to avoid adding implicit layer of validity checks into a new ad-hoc function that addresses ctor/dtor case. However, if other reviewers think that asserts are ok here and may remain as it is, I'd strongly suggest to consider better error messages, adding more context to the message to facilitate possible future testing/debugging efforts.

The assert is to make sure StTy->getElementType(2) and CS->getAggregateElement(2) below are valid and the assert aligns with assert at https://github.com/llvm/llvm-project/blob/248fba0cd806a0f6bf4b0f12979f2185f2bed111/llvm/lib/IR/Verifier.cpp#L799

I'd strongly suggest to consider better error messages

done

VyacheslavLevytskyy · 2023-12-21T13:22:35Z

lib/SPIRV/SPIRVReader.cpp

+    if (Init && IsCtorOrDtor) {
+      Initializer = dyn_cast<Constant>(transValue(Init, F, BB, false));
+      postProcGlobalCtorDtorTypeInit(Ty, Initializer);
+    }


It's confusing to have this check separated from if-statement below, and to repeat the check for IsCtorOrDtor twice, here and below.

I moved the code into transGlobalCtorDtors

VyacheslavLevytskyy · 2023-12-21T13:29:44Z

lib/SPIRV/SPIRVReader.cpp

+/// IR verifier. E.g.
+///   [1 x %0][%0 { i32 1, i8* bitcast (void ()* @ctor to i8*), i8* null }]
+/// This function removes the cast so that LLVM IR is valid.
+static void postProcGlobalCtorDtorTypeInit(Type *&Ty, Constant *&Initializer) {


The function is defined to be void, as if it would cause a side effect rather than convert one statement into another, as it actually does by reassigning an argument Initializer . It would require more time to understand the underlying logic comparing to line 1595 style of
Initializer = dyn_cast<Constant>(transValue(Init, F, BB, false));.
In my opinion, it's better to return a pointer than to change the *& argument.

VyacheslavLevytskyy · 2023-12-21T13:30:37Z

lib/SPIRV/SPIRVReader.cpp

+                         BV->getName() == "llvm.global_dtors");
+    if (Init && IsCtorOrDtor) {
+      Initializer = dyn_cast<Constant>(transValue(Init, F, BB, false));
+      postProcGlobalCtorDtorTypeInit(Ty, Initializer);


As in the comment to line 1340, it's not obvious that the function changes Initializer value. Initializer = would work better.

LU-JOHN · 2023-12-21T20:22:54Z

We may want to look at lib/SPIRV/SPIRVTypeScavenger.cpp, which was written to:

// This file implements the necessary logic to recover pointer types from LLVM
// IR for the output SPIR-V file from the opaque pointers in LLVM IR.

It would be better if the SPIRVTypeScavenger was able to get the correct type rather than adding custom code for global c/dtors.

@jcranmer-intel can you comment if this issue is something that is better handled in the type scavenger?

wenju-he · 2023-12-22T01:51:22Z

We may want to look at lib/SPIRV/SPIRVTypeScavenger.cpp, which was written to:

// This file implements the necessary logic to recover pointer types from LLVM
// IR for the output SPIR-V file from the opaque pointers in LLVM IR.

It would be better if the SPIRVTypeScavenger was able to get the correct type rather than adding custom code for global c/dtors.

@jcranmer-intel can you comment if this issue is something that is better handled in the type scavenger?

Yeah we probably need to back-port SPIRVTypeScavenger to llvm_release_140 and llvm_release_150 branches. I'm not familiar with the code in SPIRVTypeScavenger. @jcranmer-intel what's your opinion?

edit: it seems @jcranmer-intel is on holiday now.

wenju-he · 2023-12-22T06:27:29Z

lib/SPIRV/SPIRVReader.cpp

+/// casted to i8* type in \p Ty and \p Initializer, which causes error in LLVM
+/// IR verifier. E.g.
+///   [1 x %0][%0 { i32 1, i8* bitcast (void ()* @ctor to i8*), i8* null }]
+/// This function removes the cast so that LLVM IR is valid.


ctor/dtor use function pointer. This problem here is kind of special that LLVM IR verifier specifically requires that the type of the second element should be a function type rather than i8* type, see https://github.com/llvm/llvm-project/blob/248fba0cd806a0f6bf4b0f12979f2185f2bed111/llvm/lib/IR/Verifier.cpp#L798

Indeed, the issue this PR addresses might also be related to cases that a function pointer is used in the code, e.g. as an indirect function call. With the second commit of this PR, I verified that SPV from opaque pointer in tests in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/tree/main/test/extensions/INTEL/SPV_INTEL_function_pointers could be translated to LLVM IR with typed pointer using SPIRV-LLVM-Translator llvm_release_140 branch. Therefore, function pointer handling seems good.

Other than function pointer, I don't know if opaque types handling in SPIRV-LLVM-Translator llvm_release_140 branch has more issues. As @LU-JOHN mentioned, we might need the whole SPIRVTypeScavenger.

wenju-he · 2023-12-22T06:28:13Z

lib/SPIRV/SPIRVReader.cpp

+/// IR verifier. E.g.
+///   [1 x %0][%0 { i32 1, i8* bitcast (void ()* @ctor to i8*), i8* null }]
+/// This function removes the cast so that LLVM IR is valid.
+static void postProcGlobalCtorDtorTypeInit(Type *&Ty, Constant *&Initializer) {


wenju-he · 2023-12-22T06:28:22Z

lib/SPIRV/SPIRVReader.cpp

@@ -1331,6 +1331,39 @@ void SPIRVToLLVM::transFunctionPointerCallArgumentAttributes(
  }
 }

+/// When spirv is generated from LLVM IR with opapque pointer enabled and then


wenju-he · 2023-12-22T06:36:34Z

lib/SPIRV/SPIRVReader.cpp

+  unsigned NumEltsInArr = InitArr->getType()->getNumElements();
+  assert(NumEltsInArr && "array is empty");
+  auto *CS = cast<ConstantStruct>(InitArr->getAggregateElement(0u));
+  assert(CS->getType()->getNumElements() == 3 && "expect 3 elements in struct");


The assert is to make sure StTy->getElementType(2) and CS->getAggregateElement(2) below are valid and the assert aligns with assert at https://github.com/llvm/llvm-project/blob/248fba0cd806a0f6bf4b0f12979f2185f2bed111/llvm/lib/IR/Verifier.cpp#L799

I'd strongly suggest to consider better error messages

done

wenju-he · 2023-12-22T06:36:58Z

lib/SPIRV/SPIRVReader.cpp

+                         BV->getName() == "llvm.global_dtors");
+    if (Init && IsCtorOrDtor) {
+      Initializer = dyn_cast<Constant>(transValue(Init, F, BB, false));
+      postProcGlobalCtorDtorTypeInit(Ty, Initializer);


wenju-he · 2023-12-22T06:37:22Z

lib/SPIRV/SPIRVReader.cpp

+    if (Init && IsCtorOrDtor) {
+      Initializer = dyn_cast<Constant>(transValue(Init, F, BB, false));
+      postProcGlobalCtorDtorTypeInit(Ty, Initializer);
+    }


I moved the code into transGlobalCtorDtors

wenju-he · 2023-12-22T07:12:56Z

lib/SPIRV/SPIRVReader.cpp

      transGlobalCtorDtors(BV);
+    else if (BV->getStorageClass() != StorageClassFunction)
+      transValue(BV, nullptr, nullptr);


the change of order here is to fix https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/test/extensions/INTEL/SPV_INTEL_function_pointers/global_ctor_dtor.ll and https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/test/extensions/INTEL/SPV_INTEL_function_pointers/global_ctor_dtor_addrspace.ll in the case SPV from opaque pointer LLVM IR is translated to typed pointer LLVM IR.

wenju-he · 2023-12-22T07:56:54Z

We may want to look at lib/SPIRV/SPIRVTypeScavenger.cpp, which was written to:

// This file implements the necessary logic to recover pointer types from LLVM
// IR for the output SPIR-V file from the opaque pointers in LLVM IR.

It would be better if the SPIRVTypeScavenger was able to get the correct type rather than adding custom code for global c/dtors.
@jcranmer-intel can you comment if this issue is something that is better handled in the type scavenger?

Yeah we probably need to back-port SPIRVTypeScavenger to llvm_release_140 and llvm_release_150 branches. I'm not familiar with the code in SPIRVTypeScavenger. @jcranmer-intel what's your opinion?

edit: it seems @jcranmer-intel is on holiday now.

@LU-JOHN back-porting SPIRVTypeScavenger requires some efforts that probably can't be done in a short time. Can we proceed with PR at the moment since the fix is urgently needed? I can file an issue for back-porting SPIRVTypeScavenger .

wenju-he · 2023-12-23T03:21:40Z

We may want to look at lib/SPIRV/SPIRVTypeScavenger.cpp, which was written to:

// This file implements the necessary logic to recover pointer types from LLVM
// IR for the output SPIR-V file from the opaque pointers in LLVM IR.

It would be better if the SPIRVTypeScavenger was able to get the correct type rather than adding custom code for global c/dtors.
@jcranmer-intel can you comment if this issue is something that is better handled in the type scavenger?

Yeah we probably need to back-port SPIRVTypeScavenger to llvm_release_140 and llvm_release_150 branches. I'm not familiar with the code in SPIRVTypeScavenger. @jcranmer-intel what's your opinion?
edit: it seems @jcranmer-intel is on holiday now.

@LU-JOHN back-porting SPIRVTypeScavenger requires some efforts that probably can't be done in a short time. Can we proceed with PR at the moment since the fix is urgently needed? I can file an issue for back-porting SPIRVTypeScavenger .

@VyacheslavLevytskyy @LU-JOHN please also note that in main branch SPIRVTypeScavenger is only used for LLVM->SPIRV translation. The scope here is SPIRV->LLVM. So it needs efforts to evaluate if SPIRVTypeScavenger can be modified to suite this scope.
If there is no objection to the current approach, could you please review and merge? We'd like it to be merged ASAP.

LU-JOHN · 2023-12-26T06:42:47Z

We may want to look at lib/SPIRV/SPIRVTypeScavenger.cpp, which was written to:

// This file implements the necessary logic to recover pointer types from LLVM
// IR for the output SPIR-V file from the opaque pointers in LLVM IR.

It would be better if the SPIRVTypeScavenger was able to get the correct type rather than adding custom code for global c/dtors.
@jcranmer-intel can you comment if this issue is something that is better handled in the type scavenger?

Yeah we probably need to back-port SPIRVTypeScavenger to llvm_release_140 and llvm_release_150 branches. I'm not familiar with the code in SPIRVTypeScavenger. @jcranmer-intel what's your opinion?
edit: it seems @jcranmer-intel is on holiday now.

@LU-JOHN back-porting SPIRVTypeScavenger requires some efforts that probably can't be done in a short time. Can we proceed with PR at the moment since the fix is urgently needed? I can file an issue for back-porting SPIRVTypeScavenger .

@VyacheslavLevytskyy @LU-JOHN please also note that in main branch SPIRVTypeScavenger is only used for LLVM->SPIRV translation. The scope here is SPIRV->LLVM. So it needs efforts to evaluate if SPIRVTypeScavenger can be modified to suite this scope. If there is no objection to the current approach, could you please review and merge? We'd like it to be merged ASAP.

Hi @wenju-he . I see you have addressed all of @VyacheslavLevytskyy concerns. Can you look at while "In-tree build & tests / Windows" is failing?

wenju-he · 2023-12-26T08:31:17Z

Hi @wenju-he . I see you have addressed all of @VyacheslavLevytskyy concerns. Can you look at while "In-tree build & tests / Windows" is failing?

@LU-JOHN my local windows in-tree build is successful.
The fail in "In-tree build & tests / Windows" is due to wrong commit is checked out from KhronosGroup/SPIRV-Headers repo :

Checking out the ref
  "C:\Program Files\Git\bin\git.exe" checkout --progress --force -B main refs/remotes/origin/main
  branch 'main' set up to track 'origin/main'.
  Switched to a new branch 'main'
"C:\Program Files\Git\bin\git.exe" log -1 --format='%H'
'1c6bb2743599e6eb6f37b2969acc0aef812e32e3'

Linux in-tree build is checking out correct commit:

Checking out the ref
 /usr/bin/git checkout --progress --force 9b527c0fb60124936d0906d44803bec51a0200fb
  Note: switching to '9b527c0fb60124936d0906d44803bec51a0200fb'.
  
  You are in 'detached HEAD' state. You can look around, make experimental
  changes and commit them, and you can discard any commits you make in this
  state without impacting any branches by switching back to a branch.
  
  If you want to create a new branch to retain commits you create, you may
  do so (now or later) by using -c with the switch command. Example:
  
    git switch -c <new-branch-name>
  
  Or undo this operation with:
  
    git switch -
  
  Turn off this advice by setting config variable advice.detachedHead to false
  
  HEAD is now at 9b527c0 Add definitions for SPV_KHR_cooperative_matrix
/usr/bin/git log -1 --format='%H'
'9b527c0fb60124936d0906d44803bec51a0200fb'

It isn't obvious to me which part of https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/llvm_release_140/.github/workflows/check-in-tree-build.yml is wrong.

wenju-he · 2023-12-26T09:31:54Z

Hi @wenju-he . I see you have addressed all of @VyacheslavLevytskyy concerns. Can you look at while "In-tree build & tests / Windows" is failing?

@LU-JOHN my local windows in-tree build is successful. The fail in "In-tree build & tests / Windows" is due to wrong commit is checked out from KhronosGroup/SPIRV-Headers repo :

submitted an issue: #2284

LU-JOHN

LGTM

bader · 2023-12-27T17:41:36Z

@wenju-he wants it to be merged asap. @VyacheslavLevytskyy, @jcranmer-intel, please, review post-commit. @wenju-he, be ready to address post-commit comments in follow-up PRs.

wenju-he · 2023-12-28T08:42:42Z

Thank you @bader

VyacheslavLevytskyy reviewed Dec 21, 2023

View reviewed changes

move the new code into transGlobalCtorDtors

fa4372c

wenju-he commented Dec 22, 2023

View reviewed changes

wenju-he requested a review from VyacheslavLevytskyy December 22, 2023 07:15

auto

050a9c7

wenju-he mentioned this pull request Dec 26, 2023

windows github workflow in-tree-build isn't fetching correct SPIRV-Headers commit #2284

Open

LU-JOHN approved these changes Dec 26, 2023

View reviewed changes

bader merged commit e7f5440 into KhronosGroup:llvm_release_140 Dec 27, 2023
8 of 9 checks passed

wenju-he deleted the remove-global-ctor-dtor-bitcast branch December 28, 2023 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPV->LLVM] Fix global c/dtors type if SPV is from opaque type LLVM #2280

[SPV->LLVM] Fix global c/dtors type if SPV is from opaque type LLVM #2280

wenju-he commented Dec 21, 2023

wenju-he commented Dec 21, 2023

wenju-he commented Dec 21, 2023

VyacheslavLevytskyy Dec 21, 2023

wenju-he Dec 22, 2023

VyacheslavLevytskyy Dec 21, 2023

wenju-he Dec 22, 2023 •

edited

Loading

wenju-he Dec 22, 2023

VyacheslavLevytskyy Dec 21, 2023

wenju-he Dec 22, 2023

VyacheslavLevytskyy Dec 21, 2023

wenju-he Dec 22, 2023

VyacheslavLevytskyy Dec 21, 2023

wenju-he Dec 22, 2023

VyacheslavLevytskyy Dec 21, 2023

wenju-he Dec 22, 2023

LU-JOHN commented Dec 21, 2023 •

edited

Loading

wenju-he commented Dec 22, 2023 •

edited

Loading

wenju-he Dec 22, 2023

wenju-he Dec 22, 2023

wenju-he Dec 22, 2023

wenju-he Dec 22, 2023

wenju-he Dec 22, 2023

wenju-he Dec 22, 2023

wenju-he Dec 22, 2023

wenju-he commented Dec 22, 2023

wenju-he commented Dec 23, 2023

LU-JOHN commented Dec 26, 2023

wenju-he commented Dec 26, 2023 •

edited

Loading

wenju-he commented Dec 26, 2023

LU-JOHN left a comment

bader commented Dec 27, 2023

wenju-he commented Dec 28, 2023

[SPV->LLVM] Fix global c/dtors type if SPV is from opaque type LLVM #2280

[SPV->LLVM] Fix global c/dtors type if SPV is from opaque type LLVM #2280

Conversation

wenju-he commented Dec 21, 2023

wenju-he commented Dec 21, 2023

wenju-he commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenju-he Dec 22, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LU-JOHN commented Dec 21, 2023 • edited Loading

wenju-he commented Dec 22, 2023 • edited Loading

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

wenju-he commented Dec 22, 2023

wenju-he commented Dec 23, 2023

LU-JOHN commented Dec 26, 2023

wenju-he commented Dec 26, 2023 • edited Loading

wenju-he commented Dec 26, 2023

LU-JOHN left a comment

Choose a reason for hiding this comment

bader commented Dec 27, 2023

wenju-he commented Dec 28, 2023

wenju-he Dec 22, 2023 •

edited

Loading

LU-JOHN commented Dec 21, 2023 •

edited

Loading

wenju-he commented Dec 22, 2023 •

edited

Loading

wenju-he commented Dec 26, 2023 •

edited

Loading