Make error message concise.

Better error messages

Simplify by moving check in parseFunctionLikeOp

This is in preparation of the visibility related changes discussed in https://llvm.discourse.group/t/external-function-declaration-has-changed-help-needed/2357. This change disallows attributes that sym_visibility/sym_name/type etc which are parsed/inferred from other places to be present in the attribute dictionary. This way, for the follow on changes, the function parser has just one parsed visibility value (parsed as keyword private/public/nested) to consider instead of trying to also look into the parsed attribute dictionary.

Use if-else

Address feedback

Remove test case

Address feedback

Fix clang-tidy warning

River's feedback

Not really. Note that both

In D92414#2426302, @jpienaar wrote:

Could we also remove the hard coding in genTypeInterfaceMethods ? (not for this one)

The fix here is such that TableGen generated inferReturnTypes takes precedence over the one generated by the InferTypeOpInterface. Just tagging the Op with the trait is not sufficient since I assume we also want the generated inferReturnTypes. The other possible fix is to avoid generating the TableGen generated inferReturnTypes method when the Op is tagged with DeclareOpInterfaceMethods<InferTypeOpInterface> and delegate the definition of the method to the user. I've erred on the side of using the TableGen one.

Ping.

Fix nit

Attempt to resolve merge conflicts

Update comment.

AFAICT it's not possible today to hit this, so this turns out to be just defensive programming. MemRef element type is either int/float/index/complex or a vector (which itself can only have int or float elements) and all these types seems to be convertible to LLVM types without any failures. As such, I don't think we will run into this issue today, but will guard against future extensions to MemRef types.

Remove leftover check.

This addresses the nit in https://reviews.llvm.org/D90803 more generically.

This is in prep for the verification check I'll be adding to disallow public declarations. Several place (in and out-of-tree) now need to mark declarations private and its much short to use of these functions.

I already push this in, but we can revisit this if there is a better way. Could you give an example where the confusion arises? To me, the last instruction seems more relevant because that's where something is missing. Are you suggesting doing:

Use 0xdeadbeef for allocated pointer

Address feedback

Use llvm::is_contained() in parseOptionalKeyword

Review feedback

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.

Update syntax to not have quotes around visibility

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.

@mehdi_amini or @ftynse, can one you take a look as well?