Page MenuHomePhabricator

[mlir:Standard] Remove support for creating a `unit` ConstantOp
ClosedPublic

Authored by rriddle on Jan 31 2022, 1:53 PM.

Details

Summary

This is completely unused upstream, and does not really have well defined semantics
on what this is supposed to do/how this fits into the ecosystem. Given that, as part of
splitting up the standard dialect it's best to just remove this behavior, instead of try
to awkwardly fit it somewhere upstream. Downstream users are encouraged to
define their own operations that clearly can define the semantics of this.

This also uncovered several lingering uses of ConstantOp that weren't
updated to use arith::ConstantOp, and worked during conversions because
the constant was removed/converted into something else before
verification.

See https://llvm.discourse.group/t/standard-dialect-the-final-chapter/ for more discussion.

Diff Detail

Event Timeline

rriddle created this revision.Jan 31 2022, 1:53 PM
rriddle requested review of this revision.Jan 31 2022, 1:53 PM
rriddle updated this revision to Diff 404724.Jan 31 2022, 1:55 PM
rriddle edited the summary of this revision. (Show Details)
rriddle edited the summary of this revision. (Show Details)Jan 31 2022, 1:55 PM
rriddle added reviewers: jpienaar, mehdi_amini.
rriddle edited the summary of this revision. (Show Details)
rriddle edited the summary of this revision. (Show Details)
bondhugula accepted this revision.Jan 31 2022, 5:38 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/test/Dialect/Standard/invalid.mlir
4

This error message can be improved.

expected symbol ref attribute

?

This revision is now accepted and ready to land.Jan 31 2022, 5:38 PM
rriddle updated this revision to Diff 405034.Feb 1 2022, 12:03 PM
rriddle updated this revision to Diff 405038.Feb 1 2022, 12:07 PM
lattner accepted this revision.Feb 1 2022, 9:46 PM
lattner added a subscriber: lattner.

Narrowing this seems great to me.

rriddle marked an inline comment as done.Feb 2 2022, 2:09 PM
rriddle added inline comments.
mlir/test/Dialect/Standard/invalid.mlir
4

Agreed. Looks like this is coming from the parser, let me look at improving this in a followup (I see a few other cases in-tree where this is firing).

This revision was landed with ongoing or failed builds.Feb 2 2022, 2:46 PM
This revision was automatically updated to reflect the committed changes.
rriddle marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 2:46 PM