External function declaration has changed. Help needed

Great, SGTM, thanks!

Sounds perfect to me, thanks!

Thanks all! One advantage of preserving the explicit syntax will also be less test churn. There will be some preparatory changes that need to go in around SymbolTable::getSymbolVisibility before the actual changes. I’ll add all the folks here to the reviews.

Something I find a bit annoying with preserving round-trip is that it can be confusing: why is there two printing form for the same thing?

Note that that’s true even today for public visibility. There are 2 printing forms for public functions or in general any public symbol. Canonicalization is possible (which is what was originally proposed) so that if the explicit visibility happens to be default one for that symbol, it won’t be printed. It will cause test churn, but that’s manageable (both function declarations with explicit private visibility and function definitions with explicit public visibility will then be printed without any visibility). We can take that a step further so that even in the in memory IR, if the sym_visibility attribute is present, it is allowed only if it’s different than the default visibility for that symbol. I am not sure what kind of issues that might cause yet, but as long as the code is using the SymbolOpInterface methods to set visibility it should be ok. We will have to fix any code that directly adds the visibility attribute on operations to go through the SymbolOpInterface methods.

This seems like a good direction to me Rahul, now that the concept of a declaration is a thing. I would suggest that we start moving the SymbolTable::* utility functions into a detail namespace though. Now that symbol is a proper interface, we shouldn’t really have users interacting with those functions directly and should leave those as implementation details.

Thanks. I have started cleaning up some of the SymbolTable::getSymbolVisibility uses to prepare for this. But with Mehdi’s last comment, I guess we still have to finalize on the issue of whether to preserve explicit default visibility. Having 2 structural forms for the same thing is not ideal, but the structural difference should be all hidden behind in the SymbolOpInterface methods so that they behave the same way.

My slight preference is to preserve explicit default visibility as it will cause less test churn. But that’s a weak argument. @bondhugula, @nicolasvasilache, @ftynse, @clattner, @River707 please comment.

Yes I know this is an existing problem, I figured I bring it up since we’re touching this topic :slight_smile:
Your proposal of canonicalizing the in-memory representation on construction seems like a good direction to me!

In general my preference is to optimize for the long term plan, regardless of the short-term aspect of fixing the tests.

+1. I would prefer we always prefer the long-term plan, even if that causes some churn pain. (I’ve eaten a lot of churn pain myself, and I still prefer it from a maintenance perspective)

Sounds good. We should wait to hear from others before we finalize on the direction. For those who do not want to re-read the entire discussion, the gist is that each symbol will now have a defined default visibility (= isDeclaration ? private : public) and the choice is whether we want to preserve explicitly specified visibility when it matches the default visibility.

  1. If we preserve, then printer/parser/in-memory IR all have an explicit representation of this visibility (printer will print, parser will add the attribute if an explicit visibility is parsed, and IR will have the sym_visibility attribute). Downside, as @mehdi_amini pointed out, is 2 representations for the same thing.

  2. Do no preserve, then printer/parser/in-memory IR will throw out the explicit representation if it matches the default visibility. Parser will parse and drop (or should it error? i.e., no backward syntax compatibility), IR verification will fail if a Symbol operation has sym_visibility attribute with value matching the default visibility, and printer will not print it (implied by the IR verification success). If the parser does not even allow explicitly specified visibility if it matches default, then the IR becomes roundtrippable again as func private @foo() and func public @bar() { return } would not even be allowed by the parser.

Hoping to get a broader consensus here for (2) above which @River707 and @mehdi_amini seem to be in favor of. FYI @nicolasvasilache, @clattner, @bondhugula, @ftynse, @jpienaar

I would say that we should go with 2, but the user is free to explicitly specify it if they want in a .mlir file. We just won’t keep it in the output, because it is implicitly the case.

– River

I would go with 2) as well. We shouldn’t be preserving explicitly specified visibility when it matches default visibility. As an analogy (some may be taking it a bit further), (1) we don’t print identity maps on memrefs even if they were explicitly specified, (2) if you have expressions like (i + 0) or (1 + 2) in memref subscripts, affine maps, etc., they aren’t preserved. They are all constructed in the simplified form.

And we should allow the explicit specification that may match the default specification.

1 Like