This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Extend Symbol verification to reject public symbol declarations.
ClosedPublic

Authored by jurahul on Nov 13 2020, 1:02 PM.

Details

Summary

Diff Detail

Event Timeline

jurahul created this revision.Nov 13 2020, 1:02 PM
jurahul requested review of this revision.Nov 13 2020, 1:02 PM
jurahul updated this revision to Diff 305245.Nov 13 2020, 1:05 PM

Update comment.

jurahul updated this revision to Diff 305255.Nov 13 2020, 1:43 PM

Attempt to resolve merge conflicts

mehdi_amini accepted this revision.Nov 13 2020, 1:56 PM
mehdi_amini added inline comments.
mlir/docs/SymbolsAndSymbolTables.md
56

It is a bit hard to find the right language here: from a SymbolTable point of view a new symbol is defined, but it is a placeholder for an entity defined outside of the current scope.

This revision is now accepted and ready to land.Nov 13 2020, 1:56 PM
mehdi_amini added inline comments.Nov 13 2020, 1:57 PM
mlir/integration_test/Sparse/CPU/matrix-market-example.mlir
14

Actually I wonder if we shouldn't just drop it for declaration and consider it the default?

jurahul marked 2 inline comments as done.Nov 13 2020, 2:28 PM
jurahul added inline comments.
mlir/docs/SymbolsAndSymbolTables.md
56

Agreed, I am open to suggestions here. Though, the spec language does not say anywhere that symbols are "defined" in a symbol table. For example, the next section says:

A SymbolTable operation provides the container for the Symbol operations.

So there is no notion of a SymbolTable "defining" a symbol at least reading the spec language here. That being said, if this can be clarified better, we should.

mlir/integration_test/Sparse/CPU/matrix-market-example.mlir
14

I know I had brought it up earlier on discourse. Two things:

  1. This (inferring function declaration visibility to be private) is something we could do only after we fix the FuncOp parsing bug to reject empty regions. My initial quick attempt was not successful there because we need to correctly handle regions with implicit terminators, where an empty region {} is ok (and will lead the addition of the implicit terminator later). So that needs to be worked on.
  2. Once that is addressed, we can consider things, but there is open question around whether the printer should also hide the visibility if it can be inferred or be more explicit and then the round-trip behavior where inferred visibility gets printed (if printer is always explicit).

In any case, I think the parser will always accept explicit visibility when present, so for now I am proposing sticking with this.

jurahul marked 2 inline comments as done.Nov 13 2020, 2:36 PM
jurahul added inline comments.
mlir/include/mlir/IR/Function.h
92

Note, this will eventually change when we eliminate all existing uses of isExternal(). So please consider this as staging for now.