This is an archive of the discontinued LLVM Phabricator instance.

Differential D129325

[mlir] add complex type to getZeroAttr
ClosedPublic

Authored by aartbik on Jul 7 2022, 1:17 PM.

Details

Reviewers
rriddle
ftynse
springerm
Commits
rGd926b3307e2f: [mlir] add complex type to getZeroAttr
Summary

Fixes issue encountered with <sparse> complex constant
https://github.com/llvm/llvm-project/issues/56428

Diff Detail

Event Timeline

aartbik created this revision.Jul 7 2022, 1:17 PM
Herald added a reviewer: rriddle. · View Herald TranscriptJul 7 2022, 1:17 PM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: anlunx, bzcheeseman, awarzynski and 22 others. · View Herald Transcript
aartbik requested review of this revision.Jul 7 2022, 1:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2022, 1:17 PM
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
aartbik added inline comments.Jul 7 2022, 1:18 PM
mlir/lib/IR/BuiltinAttributes.cpp
1592

@rriddle : is this an acceptable solution? do zero attributes need more, like a named attribute or something like that?

aartbik added a reviewer: springerm.Jul 7 2022, 1:18 PM
rriddle requested changes to this revision.Jul 7 2022, 1:21 PM
rriddle added inline comments.
mlir/lib/IR/BuiltinAttributes.cpp
1592

Nah, this looks fine for the most part. This should just return whatever the agreed upon attribute interpretation of the type is.

1595

This will fail on integer elements.

This revision now requires changes to proceed.Jul 7 2022, 1:21 PM
aartbik added a comment.Jul 7 2022, 1:25 PM

Thanks for having such a quick look.

mlir/lib/IR/BuiltinAttributes.cpp
1595

Good point yes!

Harbormaster completed remote builds in B174245: Diff 443040.Jul 7 2022, 2:01 PM
aartbik updated this revision to Diff 443057.Jul 7 2022, 2:03 PM

int/fp as element inside complex

aartbik retitled this revision from WIP [mlir] add complex type to getZeroAttr to [mlir] add complex type to getZeroAttr.Jul 7 2022, 2:03 PM
Harbormaster completed remote builds in B174257: Diff 443057.Jul 7 2022, 2:39 PM
rriddle accepted this revision.Jul 7 2022, 3:00 PM
This revision is now accepted and ready to land.Jul 7 2022, 3:00 PM
Closed by commit rGd926b3307e2f: [mlir] add complex type to getZeroAttr (authored by aartbik). · Explain WhyJul 7 2022, 4:59 PM
This revision was automatically updated to reflect the committed changes.
aartbik added a commit: rGd926b3307e2f: [mlir] add complex type to getZeroAttr.

Revision Contents

PathSize
mlir/
lib/
IR/
12 lines
test/
Integration/
Dialect/
LLVMIR/
CPU/
16 lines

Diff 443057

mlir/lib/IR/BuiltinAttributes.cpp

Show First 20 Lines • Show All 1,583 Lines • ▼ Show 20 Lines
/// Get a zero attribute for the given attribute type. /// Get a zero attribute for the given attribute type.
Attribute SparseElementsAttr::getZeroAttr() const { Attribute SparseElementsAttr::getZeroAttr() const {
auto eltType = getElementType(); auto eltType = getElementType();
// Handle floating point elements. // Handle floating point elements.
if (eltType.isa<FloatType>()) if (eltType.isa<FloatType>())
return FloatAttr::get(eltType, 0); return FloatAttr::get(eltType, 0);
// Handle complex elements.
aartbikAuthorUnsubmitted
Done
ReplyInline Actions

@rriddle : is this an acceptable solution? do zero attributes need more, like a named attribute or something like that?

aartbik: @rriddle : is this an acceptable solution? do zero attributes need more, like a named attribute…
rriddleUnsubmitted
Not Done
ReplyInline Actions

Nah, this looks fine for the most part. This should just return whatever the agreed upon attribute interpretation of the type is.

rriddle: Nah, this looks fine for the most part. This should just return whatever the agreed upon…
if (auto complexTy = eltType.dyn_cast<ComplexType>()) {
auto eltType = complexTy.getElementType();
Attribute zero;
rriddleUnsubmitted
Not Done
ReplyInline Actions

This will fail on integer elements.

rriddle: This will fail on integer elements.
aartbikAuthorUnsubmitted
Done
ReplyInline Actions

Good point yes!

aartbik: Good point yes!
if (eltType.isa<FloatType>())
zero = FloatAttr::get(eltType, 0);
else // must be integer
zero = IntegerAttr::get(eltType, 0);
return ArrayAttr::get(complexTy.getContext(),
ArrayRef<Attribute>{zero, zero});
}
// Handle string type. // Handle string type.
if (getValues().isa<DenseStringElementsAttr>()) if (getValues().isa<DenseStringElementsAttr>())
return StringAttr::get("", eltType); return StringAttr::get("", eltType);
// Otherwise, this is an integer. // Otherwise, this is an integer.
return IntegerAttr::get(eltType, 0); return IntegerAttr::get(eltType, 0);
} }
▲ Show 20 Lines • Show All 92 Lines • Show Last 20 Lines

mlir/test/Integration/Dialect/LLVMIR/CPU/test-complex-sparse-constant.mlir

  • This file was added.
// RUN: mlir-opt %s --convert-memref-to-llvm | \
// RUN: mlir-cpu-runner -e entry -entry-point-result=void
//
// Code should not crash on the complex32 sparse constant.
//
module attributes {llvm.data_layout = ""} {
memref.global "private" constant @"__constant_32xcomplex<f32>_0" : memref<32xcomplex<f32>> =
sparse<[[1], [28], [31]],
[(1.000000e+00,0.000000e+00), (2.000000e+00,0.000000e+00), (3.000000e+00,0.000000e+00)]
> {alignment = 128 : i64}
llvm.func @entry() {
%0 = memref.get_global @"__constant_32xcomplex<f32>_0" : memref<32xcomplex<f32>>
llvm.return
}
}