Page MenuHomePhabricator

[MLIR] Change FuncOp assembly syntax to print visibility inline instead of in attrib dict.
ClosedPublic

Authored by jurahul on Nov 5 2020, 9:07 AM.

Details

Summary
  • Change syntax for FuncOp to be func "<visibility>" @name instead of printing the visibility in the attribute dictionary.
  • Since printFunctionLikeOp() and parseFunctionLikeOp() are also used by other operations, make the "inline visibility" an opt-in feature.
  • Updated unit test to use and check the new syntax.

Diff Detail

Event Timeline

jurahul created this revision.Nov 5 2020, 9:07 AM
Herald added a project: Restricted Project. · View Herald Transcript
jurahul requested review of this revision.Nov 5 2020, 9:07 AM
mehdi_amini accepted this revision.Nov 5 2020, 9:20 AM
This revision is now accepted and ready to land.Nov 5 2020, 9:20 AM
ftynse added a comment.Nov 6 2020, 2:17 PM

As a syntactic nit, we don't have to print an attribute as a quoted string. It can be printed and parsed as a keyword with parseOptionalKeyword(StringRef *), from which one then creates an attribute of any desired type using a StringSwitch. This would give func private @private_function instead, which I personally prefer. As a historic reference, I introduced the first operation with "attribute" in its syntax -- cmpi -- which I now regret because many other operations follows suit.

I don't think there's a way to do that with the declarative parses though, and we use that std.global_memref so I avoided diverging the syntax between the two. But I do agree that quotes are not that readable. I can change this to be unquoted, but then we would need to change global_memref to use the similar syntax, likely by using the custom directive. @rriddle/@mehdi_amin, what do you think? It almost seems that the SymbolTable can provide helper functions to print/parse visibility that can be reused here across ops.

jurahul updated this revision to Diff 303590.Nov 6 2020, 4:34 PM

Update syntax to not have quotes around visibility

rriddle requested changes to this revision.Nov 6 2020, 4:39 PM

We could move the visibility enum to ODS, which would automatically give the correct formatting when the attribute is used on symbol operations.

mlir/include/mlir/IR/FunctionImplementation.h
81

Is this solely a staging thing? We should just always do this.

mlir/include/mlir/IR/OpImplementation.h
146

Please do not do this, just unwrap the StringAttr at the callsite.

357

This doesn't look like it needs to be a virtual method.

511

Please do not put this on the OpAsmParser, it is completely unrelated.

This revision now requires changes to proceed.Nov 6 2020, 4:39 PM
jurahul added inline comments.Nov 6 2020, 4:53 PM
mlir/include/mlir/IR/FunctionImplementation.h
81

Yes, it's a staging think at least. There are other operations that use this functionality and I am not changing all of them. I can do this for other in-tree ones in later CL's, but need to work with other folks for out-of-tree ones. May be this is something we can make default in a few weeks after all known users have been migrated to the new syntax and we give advanced notice on discourse.

mlir/include/mlir/IR/OpImplementation.h
146

Will do.

357

Agreed. Will change.

511

The other place I can think of is SymbolTable static method. Would that work? I anticipate other op's could use it when we migrate to the new syntax.

I did think of moving the visibility enums to ODS, but I wasn't sure how operations like ModuleOp and FuncOp that are defined outside ODS will use it. Also StrEnumAttr still prints a quoted string I think, so beyond that, we would also need some kind of qualifier in the declarative assembly syntax to indicate that the custom parser needs to print it without quotes (something like noquotes($sym_visibility)). So did not go down that path.

rriddle added inline comments.Nov 6 2020, 4:56 PM
mlir/include/mlir/IR/OpImplementation.h
511

The main MLIR ideology is to keep these types of very open/very common interface points as clean as possible. Putting these in SymbolTable.h is probably fine (though ideally most users should try to use the declarative syntax). I wouldn't put them in the SymbolTable class though, we can put them in an impl namespace like these methods: https://github.com/llvm/llvm-project/blob/1ca7f055ad81346bae6fc6f8b35de306b4afe169/mlir/include/mlir/IR/OpDefinition.h#L1631

jurahul added inline comments.Nov 6 2020, 5:13 PM
mlir/include/mlir/IR/OpImplementation.h
357

Actually, I take that back. We can make it non-virtual by calling one of the parseOptionalKeyword variants per allowed keyword, but that's several virtual function calls with redundant check between then (like is current token a isCurrentTokenAKeyword). With a virtual, all this overhead can be avoided.

jurahul updated this revision to Diff 303601.Nov 6 2020, 5:47 PM

Review feedback

jurahul marked 4 inline comments as done.Nov 6 2020, 5:51 PM
jurahul added inline comments.
mlir/include/mlir/IR/FunctionImplementation.h
81

Commented on this earlier.

mlir/include/mlir/IR/OpImplementation.h
511

Moved it to SymbolTable.h with impl in SymbolTable.cpp.

jurahul updated this revision to Diff 303742.Nov 8 2020, 4:56 PM
jurahul marked an inline comment as done.

Use llvm::is_contained() in parseOptionalKeyword

rriddle accepted this revision.Nov 8 2020, 5:01 PM

We should update OpFormatGen(I can do this) to support formatting enums with a keyword instead of a StringAttr when it is possible, but this is fine for now. Thanks for cleaning this up!

mlir/include/mlir/IR/FunctionImplementation.h
81

Thanks, just want to make sure this gets followed up on.

mlir/include/mlir/IR/OpImplementation.h
357

SGTM

mlir/include/mlir/IR/SymbolTable.h
314

There is only ever one possible attribute name for this, do we need to specify this?

This revision is now accepted and ready to land.Nov 8 2020, 5:01 PM
ftynse added a comment.Nov 9 2020, 5:25 AM

We should update OpFormatGen(I can do this) to support formatting enums with a keyword instead of a StringAttr when it is possible, but this is fine for now. Thanks for cleaning this up!

This would be great, thanks!

ftynse accepted this revision.Nov 9 2020, 5:25 AM
jurahul updated this revision to Diff 303883.Nov 9 2020, 8:24 AM

Address feedback