Page MenuHomePhabricator

[mlir] Use assemblyFormat in AllocLikeOp.
ClosedPublic

Authored by csigg on Nov 2 2020, 12:27 AM.

Details

Summary

Split operands into dynamicSizes and symbolOperands.

Diff Detail

Event Timeline

csigg created this revision.Nov 2 2020, 12:27 AM
csigg requested review of this revision.Nov 2 2020, 12:27 AM
csigg edited the summary of this revision. (Show Details)Nov 2 2020, 12:27 AM
bondhugula requested changes to this revision.Nov 2 2020, 2:31 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/test/IR/memory-ops.mlir
25 ↗(On Diff #302219)

Do you need to insert a space? This would make it inconsistent with affine.apply and perhaps other things as well. Is it necessary to get the assembly format working? If it's for readability, nearly all coding styles (for eg. MLIR, TensorFlow C++ styles) don't put a space in such cases between a bracket list and the parenthetical list - for eg. in lambda definitions. The square brackets were mainly used to denote a "capture" like effect.

This revision now requires changes to proceed.Nov 2 2020, 2:31 AM
csigg added inline comments.Nov 2 2020, 5:00 AM
mlir/test/IR/memory-ops.mlir
25 ↗(On Diff #302219)

The OpFormatGen produces a space between two punctuations, unless the second one is a closing bracket or a comma.

I've tried to change that behavior to no space before any bracket, but that would require inserting explicit spaces in many other assemblyFormats. I've also tried to parse dynamicSizes and symbolOperands as a single custom group, but that inserts a space after alloc.

Does the 'no space between )[' apply almost everywhere so that we should handle that case differentely?

Maybe the best alternative is to add an empty literal that would set shouldEmitSpace = false. Basically the opposite of the space literal.

@rriddle, WDYT?

csigg added inline comments.Nov 9 2020, 4:13 AM
mlir/test/IR/memory-ops.mlir
25 ↗(On Diff #302219)

I've made affine ops consistent by adding a space too. The code change itself is small, but I needed to update 80+ tests. Maybe it's easier to support a `` literal in the assemblyFormat: D91068.

bondhugula added inline comments.Nov 9 2020, 5:58 AM
mlir/test/IR/memory-ops.mlir
25 ↗(On Diff #302219)

I think we could leave it the way it was before. If you are moving this to assembly format and it requires a space in the syntax, let's go with it. It's a bit counterintuitive for those writing MLIR test cases to know that the space there is making a difference.

csigg updated this revision to Diff 304068.Nov 10 2020, 12:19 AM

Also add space to affine_map and affine.apply etc.

bondhugula requested changes to this revision.Nov 10 2020, 10:05 AM
bondhugula added inline comments.
mlir/test/IR/memory-ops.mlir
25 ↗(On Diff #302219)

I'm sorry if it wasn't clear. I'm not in favor of adding a space for all the affine ops - it's unnecessary and counterintuitive (also inconsistent with the way affine maps defs are printed - space shouldn't be making a difference). Since you are moving just the alloc ops to the assembly format, it's fine if only the alloc op use a space in the syntax. The alloc ops and affine ops need not really be consistent since it's at the expense of other downsides.

This revision now requires changes to proceed.Nov 10 2020, 10:05 AM
csigg added inline comments.Nov 10 2020, 11:56 AM
mlir/test/IR/memory-ops.mlir
25 ↗(On Diff #302219)

Is it inconsistent with affine maps? I think I updated that too.

I don't immediately see why an extra space is counterintuitive, but I definitely see the unnecessary churn on all the lit tests, just because assemblyFormat happens to insert a space.

I will see how D91068 is going, and roll this one back to just the std.alloc change, either with or without space.

csigg updated this revision to Diff 304421.Nov 11 2020, 12:45 AM

Don't introduce extra space.

csigg edited the summary of this revision. (Show Details)Nov 11 2020, 12:47 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 11 2020, 1:27 AM
This revision was automatically updated to reflect the committed changes.

onnx-mlir (https://github.com/onnx/onnx-mlir) is currently using parseDimAndSymbolList. Is the intention for this function to not be available outside of mlir any more? I notice it no longer has any uses inside mlir either.

onnx-mlir (https://github.com/onnx/onnx-mlir) is currently using parseDimAndSymbolList. Is the intention for this function to not be available outside of mlir any more? I notice it no longer has any uses inside mlir either.

It's now private to AffineOps.cpp. Maybe the easiest would be to export it again from there. Would that work for onnx?

onnx-mlir (https://github.com/onnx/onnx-mlir) is currently using parseDimAndSymbolList. Is the intention for this function to not be available outside of mlir any more? I notice it no longer has any uses inside mlir either.

It's now private to AffineOps.cpp. Maybe the easiest would be to export it again from there. Would that work for onnx?

Yes, that should. Thanks!