Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[mlir] Convert-spirv-to-llvm Pass trigger Segmentation fault in LLVMStructType verifier #59990

Closed
Colloportus0 opened this issue Jan 13, 2023 · 8 comments
Closed

[mlir] Convert-spirv-to-llvm Pass trigger Segmentation fault in LLVMStructType verifier #59990

Colloportus0 opened this issue Jan 13, 2023 · 8 comments
Assignees
@antiagainst @Pierre-vh
Labels
crash Prefer [crash-on-valid] or [crash-on-invalid] mlir:spirv

Comments

@Colloportus0
Copy link

Colloportus0 commented Jan 13, 2023 •
edited by VoltrexKeyva
Loading

MLIR built at commit a0138390
Reproduced with:
mlir-opt --convert-spirv-to-llvm temp.mlir

temp.mlir:

module { 
  spirv.module Logical GLSL450 {
    spirv.GlobalVariable @var01_scalar bind(0, 1) {aliased} : !spirv.ptr<!spirv.struct<(!spirv.rtarray<f32, stride=4> [0])>, StorageBuffer>
    spirv.GlobalVariable @var01_vec2 bind(0, 1) {aliased} : !spirv.ptr<!spirv.struct<(!spirv.rtarray<vector<2xf32>, stride=8> [0])>, StorageBuffer>
    spirv.GlobalVariable @var01_vec4 bind(0, 1) {aliased} : !spirv.ptr<!spirv.struct<(!spirv.rtarray<vector<4xf32>, stride=16> [0])>, StorageBuffer>
  
    spirv.func @load_different_vector_sizes(%i0: i32) -> vector<4xf32> "None" {
      %c0 = spirv.Constant 0 : i32
  
      %addr0 = spirv.mlir.addressof @var01_vec4 : !spirv.ptr<!spirv.struct<(!spirv.rtarray<vector<4xf32>, stride=16> [0])>, StorageBuffer>
      %ac0 = spirv.AccessChain %addr0[%c0, %i0] : !spirv.ptr<!spirv.struct<(!spirv.rtarray<vector<4xf32>, stride=16> [0])>, StorageBuffer>, i32, i32
      %vec4val = spirv.Load "StorageBuffer" %ac0 : vector<4xf32>
  
      %addr1 = spirv.mlir.addressof @var01_scalar : !spirv.ptr<!spirv.struct<(!spirv.rtarray<f32, stride=4> [0])>, StorageBuffer>
      %ac1 = spirv.AccessChain %addr1[%c0, %i0] : !spirv.ptr<!spirv.struct<(!spirv.rtarray<f32, stride=4> [0])>, StorageBuffer>, i32, i32
      %scalarval = spirv.Load "StorageBuffer" %ac1 : f32
  
      %val = spirv.CompositeInsert %scalarval, %vec4val[0 : i32] : f32 into vector<4xf32>
      spirv.ReturnValue %val : vector<4xf32>
    }
  }
  
  
}

trace:

PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: mlir-opt --convert-spirv-to-llvm temp.mlir
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  mlir-opt                 0x00000001023fc5bc llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 56
1  mlir-opt                 0x00000001023fb624 llvm::sys::RunSignalHandlers() + 112
2  mlir-opt                 0x00000001023fcc54 SignalHandler(int) + 344
3  libsystem_platform.dylib 0x00000001a56894c4 _sigtramp + 56
4  mlir-opt                 0x0000000102a8a668 bool mlir::Type::isa<mlir::LLVM::LLVMVoidType, mlir::LLVM::LLVMLabelType, mlir::LLVM::LLVMMetadataType, mlir::LLVM::LLVMFunctionType, mlir::LLVM::LLVMTokenType, mlir::LLVM::LLVMScalableVectorType>() const + 24
5  mlir-opt                 0x0000000102a8a668 bool mlir::Type::isa<mlir::LLVM::LLVMVoidType, mlir::LLVM::LLVMLabelType, mlir::LLVM::LLVMMetadataType, mlir::LLVM::LLVMFunctionType, mlir::LLVM::LLVMTokenType, mlir::LLVM::LLVMScalableVectorType>() const + 24
6  mlir-opt                 0x0000000102a8bef8 mlir::LLVM::LLVMStructType::verify(llvm::function_ref<mlir::InFlightDiagnostic ()>, llvm::ArrayRef<mlir::Type>, bool) + 76
7  mlir-opt                 0x0000000102a8bcc8 mlir::LLVM::LLVMStructType mlir::detail::StorageUserBase<mlir::LLVM::LLVMStructType, mlir::Type, mlir::LLVM::detail::LLVMStructTypeStorage, mlir::detail::TypeUniquer, mlir::DataLayoutTypeInterface::Trait, mlir::SubElementTypeInterface::Trait, mlir::TypeTrait::IsMutable>::get<llvm::ArrayRef<mlir::Type>, bool>(mlir::MLIRContext*, llvm::ArrayRef<mlir::Type>, bool) + 76
8  mlir-opt                 0x000000010332d334 std::__1::__function::__func<std::__1::enable_if<std::is_invocable_v<mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5, mlir::spirv::StructType, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>>, std::__1::function<std::__1::optional<mlir::LogicalResult> (mlir::Type, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>)>>::type mlir::TypeConverter::wrapCallback<mlir::spirv::StructType, std::__1::enable_if<std::is_invocable_v<mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5, mlir::spirv::StructType>, std::__1::function<std::__1::optional<mlir::LogicalResult> (mlir::Type, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>)>>::type mlir::TypeConverter::wrapCallback<mlir::spirv::StructType, mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5>(mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5&&)::'lambda'(mlir::spirv::StructType, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>)>(mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5&&)::'lambda'(mlir::Type, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>), std::__1::allocator<std::__1::enable_if<std::is_invocable_v<mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5, mlir::spirv::StructType, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>>, std::__1::function<std::__1::optional<mlir::LogicalResult> (mlir::Type, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>)>>::type mlir::TypeConverter::wrapCallback<mlir::spirv::StructType, std::__1::enable_if<std::is_invocable_v<mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5, mlir::spirv::StructType>, std::__1::function<std::__1::optional<mlir::LogicalResult> (mlir::Type, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>)>>::type mlir::TypeConverter::wrapCallback<mlir::spirv::StructType, mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5>(mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5&&)::'lambda'(mlir::spirv::StructType, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>)>(mlir::populateSPIRVToLLVMTypeConversion(mlir::LLVMTypeConverter&)::$_5&&)::'lambda'(mlir::Type, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>)>, std::__1::optional<mlir::LogicalResult> (mlir::Type, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>)>::operator()(mlir::Type&&, llvm::SmallVectorImpl<mlir::Type>&, llvm::ArrayRef<mlir::Type>&&) + 776
9  mlir-opt                 0x000000010363a9e4 mlir::TypeConverter::convertType(mlir::Type, llvm::SmallVectorImpl<mlir::Type>&) + 764
10 mlir-opt                 0x000000010363f0f4 mlir::TypeConverter::convertType(mlir::Type) + 64
11 mlir-opt                 0x0000000103346d14 (anonymous namespace)::GlobalVariablePattern::matchAndRewrite(mlir::spirv::GlobalVariableOp, mlir::spirv::GlobalVariableOpAdaptor, mlir::ConversionPatternRewriter&) const + 124
12 mlir-opt                 0x0000000102f78af4 mlir::OpConversionPattern<mlir::spirv::GlobalVariableOp>::matchAndRewrite(mlir::Operation*, llvm::ArrayRef<mlir::Value>, mlir::ConversionPatternRewriter&) const + 144
13 mlir-opt                 0x000000010363ee34 mlir::ConversionPattern::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&) const + 200
14 mlir-opt                 0x000000010389bbd0 mlir::PatternApplicator::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&, llvm::function_ref<bool (mlir::Pattern const&)>, llvm::function_ref<void (mlir::Pattern const&)>, llvm::function_ref<mlir::LogicalResult (mlir::Pattern const&)>) + 1440
15 mlir-opt                 0x00000001036494b0 (anonymous namespace)::OperationLegalizer::legalize(mlir::Operation*, mlir::ConversionPatternRewriter&) + 1948
16 mlir-opt                 0x0000000103642b1c (anonymous namespace)::OperationConverter::convertOperations(llvm::ArrayRef<mlir::Operation*>, llvm::function_ref<void (mlir::Diagnostic&)>) + 928
17 mlir-opt                 0x0000000103644d18 mlir::applyPartialConversion(mlir::Operation*, mlir::ConversionTarget&, mlir::FrozenRewritePatternSet const&, llvm::DenseSet<mlir::Operation*, llvm::DenseMapInfo<mlir::Operation*, void>>*) + 80
18 mlir-opt                 0x000000010334dc20 (anonymous namespace)::ConvertSPIRVToLLVMPass::runOnOperation() + 600
19 mlir-opt                 0x00000001036074dc mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) + 420
20 mlir-opt                 0x0000000103607a0c mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::PipelineParentInfo const*) + 320
21 mlir-opt                 0x0000000103609388 mlir::PassManager::run(mlir::Operation*) + 1148
22 mlir-opt                 0x0000000103602840 performActions(llvm::raw_ostream&, bool, bool, std::__1::shared_ptr<llvm::SourceMgr> const&, mlir::MLIRContext*, llvm::function_ref<mlir::LogicalResult (mlir::PassManager&)>, bool, bool) + 504
23 mlir-opt                 0x0000000103602410 mlir::LogicalResult llvm::function_ref<mlir::LogicalResult (std::__1::unique_ptr<llvm::MemoryBuffer, std::__1::default_delete<llvm::MemoryBuffer>>, llvm::raw_ostream&)>::callback_fn<mlir::MlirOptMain(llvm::raw_ostream&, std::__1::unique_ptr<llvm::MemoryBuffer, std::__1::default_delete<llvm::MemoryBuffer>>, llvm::function_ref<mlir::LogicalResult (mlir::PassManager&)>, mlir::DialectRegistry&, bool, bool, bool, bool, bool, bool, bool)::$_0>(long, std::__1::unique_ptr<llvm::MemoryBuffer, std::__1::default_delete<llvm::MemoryBuffer>>, llvm::raw_ostream&) + 704
24 mlir-opt                 0x000000010366d02c mlir::splitAndProcessBuffer(std::__1::unique_ptr<llvm::MemoryBuffer, std::__1::default_delete<llvm::MemoryBuffer>>, llvm::function_ref<mlir::LogicalResult (std::__1::unique_ptr<llvm::MemoryBuffer, std::__1::default_delete<llvm::MemoryBuffer>>, llvm::raw_ostream&)>, llvm::raw_ostream&, bool, bool) + 656
25 mlir-opt                 0x0000000103600838 mlir::MlirOptMain(llvm::raw_ostream&, std::__1::unique_ptr<llvm::MemoryBuffer, std::__1::default_delete<llvm::MemoryBuffer>>, llvm::function_ref<mlir::LogicalResult (mlir::PassManager&)>, mlir::DialectRegistry&, bool, bool, bool, bool, bool, bool, bool) + 216
26 mlir-opt                 0x0000000103600d2c mlir::MlirOptMain(int, char**, llvm::StringRef, mlir::DialectRegistry&, bool) + 1208
27 mlir-opt                 0x000000010229f0a0 main + 108
28 dyld                     0x0000000106ad5088 start + 516
@EugeneZelenko EugeneZelenko added mlir:spirv crash Prefer [crash-on-valid] or [crash-on-invalid] and removed new issue labels Jan 13, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 13, 2023

@llvm/issue-subscribers-mlir-spirv

@kuhar
Copy link
Member

kuhar commented Jan 13, 2023

Could you take a look, @antiagainst?

@kzhuravl
Copy link
Contributor

Looks like there was no updates since Jan. Pierre, can you take a look at this one please? Thanks!

Pierre-vh added a commit to Pierre-vh/llvm-project that referenced this issue Oct 24, 2023
@Pierre-vh
[mlir][spirv] Handle failed conversions of struct elements
4020ea5 
LLVMStructTypes could be emitted with some null elements.
This caused a crash later in the LLVMDialect verifier.

Now, properly check that all struct elements were successfully
converted before passing them to the LLVMStructType ctor.

See llvm#59990
@Pierre-vh Pierre-vh mentioned this issue Oct 24, 2023
Merged
@Pierre-vh
Copy link
Contributor

I fixed the crash in #70005, now there is this error:

temp.mlir:3:5: error: failed to legalize operation 'spirv.GlobalVariable' that was explicitly marked illegal
    spirv.GlobalVariable @var01_scalar bind(0, 1) {aliased} : !spirv.ptr<!spirv.struct<(!spirv.rtarray<f32, stride=4> [0])>, StorageBuffer>
    ^
temp.mlir:3:5: note: see current operation: "spirv.GlobalVariable"() <{sym_name = "var01_scalar_descriptor_set0_binding1", type = !spirv.ptr<!spirv.struct<(!spirv.rtarray<f32, stride=4> [0])>, StorageBuffer>}> {aliased} : () -> ()

I'm not sure if the error should be happening or not. I haven't touched MLIR/SPIRV yet so I can't tell.

@kzhuravl
Copy link
Contributor

cc @arsenm, Matt do you know who can help with the error mentioned by Pierre above?

@arsenm
Copy link
Contributor

arsenm commented Oct 25, 2023

cc @arsenm, Matt do you know who can help with the error mentioned by Pierre above?

Not really, I haven't spent any time with MLIR cc @llvm/pr-subscribers-mlir-spirv

@antiagainst
Copy link
Member

I fixed the crash in #70005, now there is this error:

temp.mlir:3:5: error: failed to legalize operation 'spirv.GlobalVariable' that was explicitly marked illegal
    spirv.GlobalVariable @var01_scalar bind(0, 1) {aliased} : !spirv.ptr<!spirv.struct<(!spirv.rtarray<f32, stride=4> [0])>, StorageBuffer>
    ^
temp.mlir:3:5: note: see current operation: "spirv.GlobalVariable"() <{sym_name = "var01_scalar_descriptor_set0_binding1", type = !spirv.ptr<!spirv.struct<(!spirv.rtarray<f32, stride=4> [0])>, StorageBuffer>}> {aliased} : () -> ()

I'm not sure if the error should be happening or not. I haven't touched MLIR/SPIRV yet so I can't tell.

Thanks for the fix in #70005; it LGTM! I think the above error is due to that we haven't supported runtime arrays with strides. Specifically here: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp#L330. It's checking that the runtime array has no strides (= 0). Instead we should extend it to support natural strides of the element type for dense packed cases. For non-dense packed cases, we may need to convert the runtime array into an array of LLVM structs in order to handle the strides there.

@Pierre-vh
Copy link
Contributor

Closing as the issue is fixed. @antiagainst please open a new ticket if this is something that needs to be implemented (I don't think i am the best person to do it though)

Pierre-vh added a commit that referenced this issue Oct 30, 2023
@Pierre-vh
[mlir][spirv] Handle failed conversions of struct elements (#70005)
30ca16e 
LLVMStructTypes could be emitted with some null elements. This caused a
crash later in the LLVMDialect verifier.

We now use `convertTypes` and check that all types were successfully converted before passing them to the `LLVMStructType` constructor.

See #59990
searlmc1 pushed a commit to ROCm/llvm-project that referenced this issue Nov 30, 2023
@Pierre-vh @ronlieb
[mlir][spirv] Handle failed conversions of struct elements (llvm#70005)
49ca7b2 
LLVMStructTypes could be emitted with some null elements. This caused a
crash later in the LLVMDialect verifier.

We now use `convertTypes` and check that all types were successfully converted before passing them to the `LLVMStructType` constructor.

See llvm#59990

Change-Id: I0f0ff1f8eb939bd760e5acb7e24017061a2eeac5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@antiagainst antiagainst

@Pierre-vh Pierre-vh

Labels
crash Prefer [crash-on-valid] or [crash-on-invalid] mlir:spirv
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@arsenm @antiagainst @kuhar @EugeneZelenko @kzhuravl @Pierre-vh @llvmbot @Colloportus0