Skip to content

[mlir][arith] Refine the verifier for arith.constant #86178

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
banach-space merged 2 commits into from
Apr 8, 2024
Merged

[mlir][arith] Refine the verifier for arith.constant #86178

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed1eb3a
[mlir][arith] Refine the verifier for arith.constant
banach-space Mar 21, 2024
f8532c3
fixup! [mlir][arith] Refine the verifier for arith.constant
banach-space Mar 22, 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: 8 additions & 0 deletions mlir/lib/Dialect/Arith/IR/ArithOps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,14 @@ LogicalResult arith::ConstantOp::verify() {
return emitOpError(
"value must be an integer, float, or elements attribute");
}

// Intializing scalable vectors with elements attribute is not supported
// unless it's a vector splot.
auto vecType = dyn_cast<VectorType>(type);
auto val = dyn_cast<DenseElementsAttr>(getValue());
if ((vecType && val) && vecType.isScalable() && !val.isSplat())
Copy link
Member

@kuhar kuhar Mar 21, 2024

Choose a reason for hiding this comment

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

nit: you can simplify this a bit

Suggested change
auto val = dyn_cast<DenseElementsAttr>(getValue());
if ((vecType && val) && vecType.isScalable() && !val.isSplat())
if (vecType && vecType.isScalable() && !isa<SplatElementsAttr>(getValue()))

Copy link
Member

@kuhar kuhar Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

btw, have you thought of having a convenience type for scalable vectors? I mean ScalableVectorType that is VectorType with a classoff function that checks for the scalable bit, similar to how we have spirv::ScalarType that allows ints and floats. Then you could use that for isa<ScalableVectorType>(...) checks.

Copy link
Contributor Author

@banach-space banach-space Mar 22, 2024

Choose a reason for hiding this comment

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

Interesting idea - I've not thought about it, no. Thanks for the suggestion! Though isa<ScalableVectorType>(...) would be longer than vecType.isScalable() 🤷🏻

In general, we've been discussing how to better represent "scalable" vectors in MLIR for a while now. There was also a long discussion on "dynamic" vector type too:

I know that @dcaballe has been working on an updated class hierarchy for vectors. IIRC, that's not at the top of the priority list ATM, but it would be good to revisit it soon 😅

Copy link
Member

@kuhar kuhar Mar 22, 2024

Choose a reason for hiding this comment

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

Interesting idea - I've not thought about it, no. Thanks for the suggestion! Though isa(...) would be longer than vecType.isScalable() 🤷🏻

only if you have a vector type already. if the starting point is just Type, you could do auto scalableTy = dyn_cast<ScalableVectorType>(myType):

Copy link
Member

@kuhar kuhar Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In general, we've been discussing how to better represent "scalable" vectors in MLIR for a while now. There was also a long discussion on "dynamic" vector type too:

What I suggested above is just a convenience type that doesn't affect the hierarchy in any way, similar to spirv::ScalarType or SplatElementsAttr.

Copy link
Contributor Author

@banach-space banach-space Mar 25, 2024

Choose a reason for hiding this comment

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

I've not tried it yet, but intend to do so this week.

One worry that I have is naming - we may want to keep ScalableVectorType for when we do update the hierarchy. This is worth trying nonetheless!

Copy link
Contributor

@dcaballe dcaballe Apr 3, 2024

Choose a reason for hiding this comment

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

I think this could be a good stepping stone but let's discuss further to make sure we don't make the potential big change more complicated. Revisiting VectorType is something I would like to do hopefully this month.

Copy link
Contributor Author

@banach-space banach-space Apr 8, 2024

Choose a reason for hiding this comment

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

@kuhar I went ahead and landed this to unblock myself elsewhere. I've also tried your suggestion and was positively surprised how straightforward that was:

@dcaballe I've prepared #87986 as one point in the discussion on VectorType refactor. Mainly so that we have a reference. However, having implemented it I feel that it would indeed be a good stepping stone 😅

return emitOpError(
"using elements attribute to initialize a scalable vector");
return success();
}

Expand Down
8 changes: 8 additions & 0 deletions mlir/test/Dialect/Arith/invalid.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,14 @@ func.func @func_with_ops() {

// -----

func.func @func_with_ops() {
^bb0:
// expected-error@+1 {{op failed to verify that result type has i1 element type and same shape as operands}}
%c = arith.constant dense<[0, 1]> : vector<[2] x i32>
}

// -----

func.func @invalid_cmp_shape(%idx : () -> ()) {
// expected-error@+1 {{'lhs' must be signless-integer-like, but got '() -> ()'}}
%cmp = arith.cmpi eq, %idx, %idx : () -> ()
Expand Down