Page MenuHomePhabricator

Introduced CallOp Dialect Conversion
ClosedPublic

Authored by rsuderman on Mar 17 2020, 2:13 PM.

Details

Summary

Utility to perform CallOp Dialect conversion, specifically handling cases where
an argument type has changed and the corresponding CallOp needs to be updated.

Diff Detail

Event Timeline

rsuderman created this revision.Mar 17 2020, 2:13 PM

Looks good, just have a question about the location.

mlir/include/mlir/Dialect/StandardOps/Transforms/DialectConversion.h
1 ↗(On Diff #250899)

Would this make more sense in the Conversion directory? Say mlir/Conversion/StandardToStandard?

9 ↗(On Diff #250899)

This seems like it should be updated, likely should just mention that it contains patterns for lowering within standard dialect.

17 ↗(On Diff #250899)

I think we can trim all of these and just use forward declarations.

29 ↗(On Diff #250899)

operand and result types?

mlir/lib/Dialect/StandardOps/Transforms/DialectConversion.cpp
1 ↗(On Diff #250899)

Missing a file header here.

6 ↗(On Diff #250899)

nit: prefer using namespace mlir in .cpp files

rsuderman updated this revision to Diff 251211.Mar 18 2020, 4:33 PM

Update for rriddle@ comments

rsuderman updated this revision to Diff 251213.Mar 18 2020, 4:35 PM
rsuderman marked 4 inline comments as done.

Missed updated comment.

rsuderman marked 3 inline comments as done.Mar 18 2020, 4:35 PM
rsuderman added inline comments.
mlir/include/mlir/Dialect/StandardOps/Transforms/DialectConversion.h
1 ↗(On Diff #250899)

Works for me.

rsuderman updated this revision to Diff 251214.Mar 18 2020, 4:37 PM

Second comment update.

rsuderman updated this revision to Diff 251215.Mar 18 2020, 4:40 PM

Updated comment in DialectConversion.cpp

rriddle accepted this revision.Mar 18 2020, 5:02 PM

LGTM after resolving last comments.

mlir/include/mlir/Conversion/StandardToStandard/StandardToStandard.h
2

This needs to be updated.

mlir/lib/Conversion/StandardToStandard/DialectConversion.cpp
1 ↗(On Diff #251215)

Can we name this file the same as the header?

This revision is now accepted and ready to land.Mar 18 2020, 5:02 PM
Harbormaster completed remote builds in B49661: Diff 251215.
Harbormaster completed remote builds in B49660: Diff 251214.
rsuderman updated this revision to Diff 251234.Mar 18 2020, 5:53 PM

Updated for final comments

Harbormaster completed remote builds in B49668: Diff 251234.
This revision was automatically updated to reflect the committed changes.