[PSA] StructAttr is deprecated and is being removed

StructAttr is deprecated and will be removed on June 20. Since StructAttr is a dictionary attribute with wrappers, its implementation is very inneficient. StructAttr is redundant with AttrDef.

I already have patches that replace its use in upstream dialects (you’re welcome! :slight_smile: ). E.g. replacing StructAttr in TOSA: ⚙ D127370 [mlir][tosa] Replace StructAttrs with AttrDefs.

Before:

def MyStructAttr : StructAttr<"Struct", MyDialect, 
                              [StructFieldAttr<"foo", I32Attr>]>;

{my_attr = {foo = 5 : i32}}

After:

def MyStructAttr : AttrDef<MyDialect, "Struct"> {
  let parameters = (ins "int":$foo);
  let mnemonic = "struct";
  let assemblyFormat = "`<` struct(params) `>`";
}

{my_attr = #my_dialect.struct<foo = 5>}

Nice. I’m previously been hit by this appealing footgun, +1 on it going away.

I’m glad that this is going away, thank you :slight_smile: But… I really don’t like the pages of warning spam I now get when building MLIR:

/usr/local/google/home/gcmn/src/iree/third_party/llvm-project/mlir/include/mlir/Dialect/SPIRV/IR/TargetAndABI.td:42:1: warning: Using deprecated def `anonymous_1239`
def SPV_CooperativeMatrixPropertiesNVAttr :
^
note: StructAttr is being removed on June 20

I think we’ve hit this sort of thing before with other deprecations. It would be nice if we could figure out a good way to notify users without overflowing their output buffers.

In this particular case, I think it would’ve been better to replace all the upstream uses of StructAttr before marking it as deprecated. Right now, users are getting warnings for things that aren’t even from their project.

1 Like

I was wondering if we could limit to one warning per file?

We could yes.

This has been reported as deprecated in the forum a couple of weeks (months?) ago, highlighting it while building is most visible mechanism (indeed could be too much, I found it useful to be able to do multiple fixes in one go).

Right but the deprecated usages are in MLIR core. Highlighting these is not a good experience for downstream users

Actually warnings can be too easy to miss, instead we could make it an error but with a flag that suppress the error --allow-deprecated-struct-attr-will-be-removed-on-6-20-2002 so that users must take an action to either fix their uses or add the flag in their CMake files for now and work on the migration, but they won’t be taken by surprise when it gets removed :slight_smile:
(of course we should prioritize migrating core users first!)

1 Like

I think the point is that we should only enable warnings for ecosystem users when MLIR core is clean. Having the MLIR build generate warnings by default seems weird?

6 Likes

Oh, yeah, you’re right. I should’ve pushed the deprecation marker after the uses upstream have been removed. They were only a day or two apart so hopefully it didn’t cause too much annoyance :blush:

Yeah, unfortunately we synced our project with LLVM in that gap. Not a huge deal, but I do think it would be nice to come up with better processes for deprecating things. Like setting deprecation warnings as errors in core would enforce that there was no usage of deprecated things within core (although I’ve noticed that it seems like few bots actually build with -Werror), but that might be hard to scope to only being within the project itself and not something that another component has deprecated (e.g. LLVM core libraries or whatever). I think part of the problem is that you really want two switches when deprecating something: “don’t add new usages of this” and “this is going away soon, so you need to remove your usages of it”. The conflation of the too encourages adding the deprecation warning earlier than desirable for the latter case in an effort to indicate the former, I think

Please can we improve the message “note: StructAttr is being removed on June 20”.
A user looking at that will say, “So what? How do I fix it? Who knows?”
If the message also includes a related ticket number. e.g. D127370
The poor user will be less clueless.