Split operands into dynamicSizes and symbolOperands.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/test/IR/memory-ops.mlir | ||
---|---|---|
25 | 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. |
mlir/test/IR/memory-ops.mlir | ||
---|---|---|
25 | 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? |
mlir/test/IR/memory-ops.mlir | ||
---|---|---|
25 | 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. |
mlir/test/IR/memory-ops.mlir | ||
---|---|---|
25 | 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. |
mlir/test/IR/memory-ops.mlir | ||
---|---|---|
25 | 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. |
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?
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.