Skip to content

Crash when lowering multiple dimention array #957

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.

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

Closed
ChuanqiXu9 opened this issue Oct 10, 2024 · 0 comments · Fixed by #961
Closed

Crash when lowering multiple dimention array #957

ChuanqiXu9 opened this issue Oct 10, 2024 · 0 comments · Fixed by #961

Comments

@ChuanqiXu9
Copy link
Member

Reproducer:

// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o %t.ll
// RUN: FileCheck --input-file=%t.ll %s -check-prefix=LLVM

unsigned char table[10][3] =
{
    {1,0},
    {7,6,5},
};

the interesting part is, if we change the first dimension 10 to any number lower, it won't crash.

The crash information comes from:

return mlir::DenseElementsAttr::get(mlir::RankedTensorType::get(dims, type),
llvm::ArrayRef(values));

where the MLIR will assert the size of values will be the same with tensor. The shape of the tensor is correct. However, we will insert values to values in

template <typename AttrTy, typename StorageTy>
void convertToDenseElementsAttrImpl(mlir::cir::ConstArrayAttr attr,
llvm::SmallVectorImpl<StorageTy> &values) {
if (auto stringAttr = mlir::dyn_cast<mlir::StringAttr>(attr.getElts())) {
if (auto arrayType = mlir::dyn_cast<mlir::cir::ArrayType>(attr.getType())) {
for (auto element : stringAttr) {
auto intAttr = mlir::cir::IntAttr::get(arrayType.getEltType(), element);
values.push_back(mlir::dyn_cast<AttrTy>(intAttr).getValue());
}
return;
}
}
auto arrayAttr = mlir::cast<mlir::ArrayAttr>(attr.getElts());
for (auto eltAttr : arrayAttr) {
if (auto valueAttr = mlir::dyn_cast<AttrTy>(eltAttr)) {
values.push_back(valueAttr.getValue());
} else if (auto subArrayAttr =
mlir::dyn_cast<mlir::cir::ConstArrayAttr>(eltAttr)) {
convertToDenseElementsAttrImpl<AttrTy>(subArrayAttr, values);
if (mlir::dyn_cast<mlir::StringAttr>(subArrayAttr.getElts()))
fillTrailingZeros(subArrayAttr, values);
} else if (auto zeroAttr = mlir::dyn_cast<mlir::cir::ZeroAttr>(eltAttr)) {
unsigned numStoredZeros = 0;
auto nestTy =
getNestedTypeAndElemQuantity(zeroAttr.getType(), numStoredZeros);
values.insert(values.end(), numStoredZeros,
getZeroInitFromType<StorageTy>(nestTy));
} else {
llvm_unreachable("unknown element in ConstArrayAttr");
}
}
// Only fill in trailing zeros at the local cir.array level where the element
// type isn't another array (for the mult-dim case).
fillTrailingZeros(attr, values);
}
and it looks like the logics are somehow not correct so that it didn't generate values with enough elements.

I think we should resize values in the first place and assign values to the corresponding slot.

keryell pushed a commit to keryell/clangir that referenced this issue Oct 19, 2024
…ly (llvm#961)

Close llvm#957

the previous algorithm to convert a multiple dimension array to a tensor
is: fill the value one by one and fill the zero values in conditions.
And it has some problems handling the multiple dimension array as above
issue shows so that the generated values are not in the same shape with
the original array.

the new algorithm here is, full fill the values ahead of time with the
correct element size and full fill the values to different slots and we
only need to maintain the index to write.

I feel the new version has better performance (avoid allocation) and
better readability slightly.
lanza pushed a commit that referenced this issue Nov 5, 2024
…ly (#961)

Close #957

the previous algorithm to convert a multiple dimension array to a tensor
is: fill the value one by one and fill the zero values in conditions.
And it has some problems handling the multiple dimension array as above
issue shows so that the generated values are not in the same shape with
the original array.

the new algorithm here is, full fill the values ahead of time with the
correct element size and full fill the values to different slots and we
only need to maintain the index to write.

I feel the new version has better performance (avoid allocation) and
better readability slightly.
lanza pushed a commit that referenced this issue Mar 18, 2025
…ly (#961)

Close #957

the previous algorithm to convert a multiple dimension array to a tensor
is: fill the value one by one and fill the zero values in conditions.
And it has some problems handling the multiple dimension array as above
issue shows so that the generated values are not in the same shape with
the original array.

the new algorithm here is, full fill the values ahead of time with the
correct element size and full fill the values to different slots and we
only need to maintain the index to write.

I feel the new version has better performance (avoid allocation) and
better readability slightly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant