Page MenuHomePhabricator

[MLIR] Shape to Standard dialect lowering
ClosedPublic

Authored by frgossen on Jun 3 2020, 6:04 AM.

Details

Summary

Add new pass to lower operations from the shape to the std dialect.
The conversion applies only to the size_to_index and to the index_to_size
operations.
Patterns will be added as needed.

Diff Detail

Event Timeline

frgossen created this revision.Jun 3 2020, 6:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2020, 6:05 AM
pifon2a added inline comments.Jun 3 2020, 6:24 AM
mlir/lib/Conversion/ShapeToStandard/ShapeToStandard.cpp
22

nit: no need for the empty line

23

this class is not really needed. You can just move addLegalDialect<scf::SCFDialect, StandardOpsDialect>(); together with addLegalOp below.

32

nit: no need for the empty line

35

nit: no need for the empty line

50

nit: no need for the empty line

53

and here

64

and even here

74

nit: and here

79

maybe just inline it in addConversion?

81

why do you need to initialize it with nullptr?

100

merge this with the first addLegalOp call ?

121

you have two patterns here, but one test.

mlir/test/Conversion/ShapeToStandard/shape-to-standard.mlir
10

I don't think this comment brings a lot. Maybe smth like:

// CHECK-LABEL: @size_to_index
func @size_to_index(%size : !shape.size) -> !shape.size {
  return %size : !shape.size
}
// CHECK-SAME: ([[SIZE:%.*]]: index)
// CHECK: return [[SIZE]] : index
frgossen updated this revision to Diff 268217.Jun 3 2020, 8:36 AM
frgossen marked 13 inline comments as done.

Fix nits and add test cases

frgossen marked an inline comment as done.Jun 3 2020, 8:40 AM

Thanks

mlir/lib/Conversion/ShapeToStandard/ShapeToStandard.cpp
81

It's not really needed here, rather a habit that prevents uninitialised pointers in general.
I'm happy to make it uninitialised here.

100

Good eye, thanks

121

It's actually worse, I had no test for any of them

mlir/test/Conversion/ShapeToStandard/shape-to-standard.mlir
10

Updated the test cases and comments. Hope this makes it clear.

pifon2a accepted this revision.Jun 3 2020, 8:54 AM
pifon2a added inline comments.
mlir/lib/Conversion/ShapeToStandard/ShapeToStandard.cpp
62

since you're copying ctx ptr into the lambda, you probably don't need it here.

This revision is now accepted and ready to land.Jun 3 2020, 8:54 AM
frgossen updated this revision to Diff 268227.Jun 3 2020, 9:15 AM

Remove unused member

This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.Jun 3 2020, 10:12 AM
mlir/include/mlir/Conversion/ShapeToStandard/ShapeToStandard.h
12

Can you trim this header? It doesn't seem necessary.

mlir/lib/Conversion/ShapeToStandard/ShapeToStandard.cpp
17

using namespace mlir;

93

This isn't llvm style, please use explicit resolution: mlir:: populateShapeToStandardConversionPatterns

Same below.