Skip to content

Fix crash when using when using --finalize-memref-to-llvm #112433

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

Merged
merged 7 commits into from
Nov 8, 2024
Merged

Fix crash when using when using --finalize-memref-to-llvm #112433

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b8780fb
Fix crash when using when using --finalize-memref-to-llvm
siddhesh195 Oct 15, 2024
b3eba80
Resolve test failure due to incorrect usage of getInt()
siddhesh195 Oct 16, 2024
709f0ce
Update mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
siddhesh195 Oct 18, 2024
419342b
Emit error message on the operation
siddhesh195 Oct 18, 2024
46719a2
Merge branch 'main' of https://github.com/siddhesh195/llvm-project
siddhesh195 Oct 18, 2024
b039d7a
Fix clang-format error
siddhesh195 Oct 30, 2024
9904acb
Merge branch 'main' into main
siddhesh195 Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion mlir/lib/Conversion/LLVMCommon/Pattern.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,14 @@ bool ConvertToLLVMPattern::isConvertibleAndHasIdentityMaps(

Type ConvertToLLVMPattern::getElementPtrType(MemRefType type) const {
auto addressSpace = getTypeConverter()->getMemRefAddressSpace(type);
if (failed(addressSpace))
if (failed(addressSpace)) {
emitError(UnknownLoc::get(type.getContext()),
"conversion of memref memory space ")
<< type.getMemorySpace()
<< " to integer address space "
"failed. Consider adding memory space conversions.";
return {};
}
return LLVM::LLVMPointerType::get(type.getContext(), *addressSpace);
}

Expand Down
7 changes: 5 additions & 2 deletions mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,8 +527,11 @@ LLVMTypeConverter::getMemRefAddressSpace(BaseMemRefType type) const {
return failure();
if (!(*converted)) // Conversion to default is 0.
return 0;
if (auto explicitSpace = llvm::dyn_cast_if_present<IntegerAttr>(*converted))
return explicitSpace.getInt();
if (auto explicitSpace = llvm::dyn_cast_if_present<IntegerAttr>(*converted)) {
if (explicitSpace.getType().isIndex() ||
Copy link
Contributor

@krzysz00 krzysz00 Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead change this to get the integer out of uiN attributes instead?

If not, this is reasonably approved

Copy link
Contributor Author

@siddhesh195 siddhesh195 Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, there currently is no separate attributes defined for signless and signed integer in BuiltinAttributes and IntegerAttr represents both signless and signed. BuiltinAttributes

Copy link
Contributor Author

@siddhesh195 siddhesh195 Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the reviews. If the PR looks good, can someone help merge because I don't have write access

Copy link
Contributor

@krzysz00 krzysz00 Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll run CI and land this, thanks for helping!

explicitSpace.getType().isSignlessInteger())
return explicitSpace.getInt();
}
return failure();
}

Expand Down
10 changes: 10 additions & 0 deletions mlir/test/Conversion/MemRefToLLVM/invalid-uint.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// RUN: mlir-opt %s -finalize-memref-to-llvm 2>&1 | FileCheck %s
// Since the error is at an unknown location, we use FileCheck instead of
// -veri-y-diagnostics here

// CHECK: conversion of memref memory space 1 : ui64 to integer address space failed. Consider adding memory space conversions.
// CHECK-LABEL: @invalid_int_conversion
func.func @invalid_int_conversion() {
%alloc_0 = memref.alloc() {alignment = 64 : i64} : memref<10xf32, 1 : ui64>
return
}
Loading