Hello,
It seems that the function declaration syntax has changed, and that each declaration of an external function must be assigned to a module.
Can you point me to the relevant examples or documentation?
The two cases I’m interested in are:
BTW, what is the reason for no longer allowing function declarations with global visibility?
Best,
Dumitru
Ok, after some research it seems that I have to declare all declarations of functions as “private”. But is this really necessary? Isn’t this implicit, that declarations only have the scope of the current module?
This also seems weird to me. Generally we want the default to work without syntactic overhead like this.
This was discussed here: RFC: Symbol definition/declaration x visibility checks
I also suggested this in the review and @jurahul mentioned that we can add this later after another issue is fixed in the parser (it has been fixed since then): https://reviews.llvm.org/D91456#inline-851955
Yes, inferring the default required fixing FuncOp parsing to reject empty regions, which has landed last week (https://reviews.llvm.org/D91886). I will now work on changing the FuncOp parser to infer default private visibility for function declarations so that an explicit “private” is not needed.
Quick question: Do we want the printer to also hide “private” when the function being printed is a declaration and hence has a default private visibility? This will cause a lot of churn in tests, since CHECK’s have to be updated. That is fine, but just want to get a consensus before I update a large number of tests. So a private function declaration can come in 3 forms:
func @foo() // private inferred
func private @foo() // explicit visibility
func @foo() attributes { sym_visibility = "private" } // legacy syntax
and in all cases will be printed as
func @foo()
Does that seems reasonable? Note that if it was not a declaration:
func @foo() {
return
}
The default visibility is public. Is that a cause of concern?
I’d be in favor of leaving it out (implicit) for declarations, but @ftynse had concerns about it so I’d like to hear from him first.
It would be nice if it roundtripped without modification in both the implicit and explicit private cases.
That would require one to record and track how it was parsed through to printing. Given textual form is for debugging, I don’t know if making that tracked in IR is good tradeoff.
I’d be pro not printing if default, matches other default printing behavior. I believe the generic form always would print, but the pretty form only optionally.
That would be confusing to me: I’d expect same default visibility for both. I would not consider that declaration and definition have different default visibility.
I would have expected that the underlying representation / attribute accepts a null / empty value and interprets that as private ?
It’s really annoying and weird to have to keep adding private for all external function declarations - I really can’t see what rationale would justify this well.
The rational to add it would be to be explicit about what the declaration means and be closer to the in-memory representation, i.e. not having:
func @foo() // implicitly private
func @bar() { // implicitly public
...
}
The alternative could be to make private the default in both cases and require an explicit public for function definitions (but that would break many users right now).
There is no such mechanism, and it’d be anti-canonicalization to have to keep two in-memory form for the same semantics. Doing this to preserve the round-trip of the custom printer does not seem usual to me, have we done this somewhere already?
So I guess there are 2 issues to sort through:
Should default visibility be same or different between declarations and definitions?
Without agreeing on this, the rest of the discussion is a moot point. Currently, SymbolTable::getSymbolVisibility returns Public if the visibility attribute is not present, so all symbols have a default public visibility. And with that definition, explicit and implicit public visibility are both roundtrippable. To infer default private visibility for function declarations, the lack of any visibility attribute would need to imply different default visibility for declarations and definitions (unless we also want to change default visibility of all functions to private, which I don’t think we want do).
Should an explicit private be roundtrippable?
That could be done with existing attributes (the single sym_visibility attribute) if we change the SymbolOpInterface::getVisibility to allow operations to define their default visibility if the attribute is not present. So:
InterfaceMethod<"Gets the visibility of this symbol.",
"mlir::SymbolTable::Visibility", "getVisibility", (ins), [{}],
/*defaultImplementation=*/[{
return mlir::SymbolTable::getSymbolVisibility(this->getOperation());
}]
>,
will change to something like:
InterfaceMethod<"Gets the visibility of this symbol.",
"mlir::SymbolTable::Visibility", "getVisibility", (ins), [{}],
/*defaultImplementation=*/[{
return mlir::SymbolTable::getSymbolVisibility(this->getOperation(), getDefaultVisibility());
}]
>,
where getDefaultVisibility() will be a new SymbolOpInterface method that will determine the inferred visibility of a given instance of a Symbol operation if there is no visibility attribute. Its default implementation will return “public” to match the existing behavior.
Not sure what you mean: there is such a thing as “OptionalAttr”, I see GlobalMemRefOp using that in “std”. I’d expect the absence of that attr to not print anything.
Yes, sym_visibility is essentially that optional attribute. We just need to parameterize the default visibility value when this optional attribute is absent to get the desired round trip behavior.
@nicolasvasilache Rahul explained above, the absence of the attribute implies “public”: this is not something we can really specialized based on declaration vs definition.
Assuming the custom parser for function declaration is changed to automatically set the attribute to private, the two declarations are parsed the same way:
func @foo()
func private @bar()
On printing we can print or omit the “private”, but we have to pick one. If you want to roundtrip these syntactically (and not just semantically) we need to know if the “private” was explicitly specified in the text and this information can’t be preserved without an extra attribute (this is what @jpienaar was describing above I believe)
I am suggesting we do this exactly in the SymbolOpInterface. Let instances of operations define what their default/inferred visibility is when the optional sym_visibility attribute is absent and then we can get round trip behavior where explicit “private” is printed as “private”…
Collecting the feedback above, here’s my proposal:
We need to agree that if we want visibility to be inferred for functions (and similar rules would
probably apply for other symbols), then
func @foo() // private visibility inferred
func @bar() { return } // public visibility inferred
is acceptable. I don’t see how we would do otherwise.
Symbol operations already need to say whether its a declaration or definition. We will define a symbols default visibility to be public if its a definition and private if its a declaration.
A Symbol operation can then be in either of the 2 states as follows:
sym_visibility attribute attached, in which case the visibility is explicitly specified. This visibility will be printed by the printer and the parser will attach this attribute if visibility was explicitly specified in the syntax. This gives consistent round-trip behavior for explicitly specified visibility.sym_visibility attribute attached, in which case the printer will not print it and parser will not attach the attribute if visibility was not explicitly specified in the syntax. If we call getVisibility for such a symbol, today it always return public. We will change that to return isDeclaration() ? private : public. This will implement the inferred public/private behavior.Please let me know if this proposal works.
Looks good to me! Thanks 