This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Fix ConstantOp verifier
ClosedPublic

Authored by marbre on Mar 8 2021, 1:35 PM.

Details

Summary

This restricts the attributes to integers for constants of type
IndexType. So far an attribute like StringAttr as in

%c1 = constant "" : index

is valid.

Diff Detail

Event Timeline

marbre created this revision.Mar 8 2021, 1:35 PM
marbre requested review of this revision.Mar 8 2021, 1:35 PM
marbre updated this revision to Diff 329285.Mar 9 2021, 3:53 AM

Updating D98216: [mlir] Fix ConstantOp verifier

marbre updated this revision to Diff 329293.Mar 9 2021, 4:30 AM

Updating D98216: [mlir] Fix ConstantOp verifier

marbre updated this revision to Diff 329294.Mar 9 2021, 4:34 AM

Updating D98216: [mlir] Fix ConstantOp verifier

marbre edited the summary of this revision. (Show Details)Mar 9 2021, 4:35 AM

Can you add a test?

Please add a test.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1215

Can you just move this under the IntegerAttr case below?

Can you add a test?

Done :)

marbre added inline comments.Mar 11 2021, 11:26 AM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1215

Do you propose to swap if (type.isa<IndexType>()) with if (!value.isa<IntegerAttr>()) or to integrate it into if (auto intAttr = value.dyn_cast<IntegerAttr>()) (line 1217) check? I think the latter is not possible.

rriddle added inline comments.Mar 11 2021, 11:31 AM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1208–1211

This could also move into the IntegerAttr check, BoolAttr is now just a special case of IntegerAttr(it used to be different).

1215

Why can't you just do:

if (auto intAttr = value.dyn_cast<IntegerAttr>()) {
  if (type.isa<IndexType>())
    return success();

  ...
}
marbre updated this revision to Diff 330051.Mar 11 2021, 12:30 PM

Address River's comment

marbre marked an inline comment as done.Mar 11 2021, 12:31 PM

All done.Address River's comment

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1215

Got your pint!

marbre edited the summary of this revision. (Show Details)Mar 11 2021, 12:32 PM
marbre marked 2 inline comments as done.
mehdi_amini accepted this revision.Mar 11 2021, 12:35 PM
This revision is now accepted and ready to land.Mar 11 2021, 12:35 PM
This revision was automatically updated to reflect the committed changes.