llvm / llvm/llvm-project

[mlir] SymbolUserAttrInterface verification only sees top-level discardable attributes

Open
#218,003 2 comments 0 reactions 0 assignees View on GitHub
mlir
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

`SymbolUserAttrInterface` (https://github.com/llvm/llvm-project/pull/153206) is only verified for top-level attributes within the discardable dictionary, consider this variant of that PR's negative test:

```mlir
module {
func.func @test() attributes {symbol_refs = [#test.symbol_ref_attr<@non_existent_symbol>]} { return }
}
```

On MLIR built today (~ToT), `mlir-opt` on the above produces no errors. However, remove the `[]` and drop the plural and we get the test currently in tree:
```mlir
module {
func.func @test() attributes {symbol_ref = #test.symbol_ref_attr<@non_existent_symbol>} { return }
}
```
which produces the following diagnostic:

```
symattr.mlir:2:3: error: 'func.func' op TestSymbolRefAttr::verifySymbolUses: '@non_existent_symbol' does not reference a valid symbol
func.func @test() attributes {symbol_ref = #test.symbol_ref_attr<@non_existent_symbol>} { return }
^
symattr.mlir:2:3: note: see current operation:
"func.func"() <{function_type = () -> (), sym_name = "test"}> ({
"func.return"() : () -> ()
}) {symbol_ref = #test.symbol_ref_attr<@non_existent_symbol>} : () -> ()
```

Machinery like `walkSymbolRefs` does walk all attributes recursively; I was surprised to find this discrepancy.

Related: #198435 (since reverted, #217959) went a different way looking for verification candidates.

While we're working towards an effective verification story there, I wanted to start a thread separately to discuss the current design point.

The `SymbolUserAttrInterface` purpose is verification, but it being ignored when not top-level is not part of its contract AFAICT.

Contributor guide

Open the contributing guide

Research direction

Start by reproducing both MLIR snippets with mlir-opt, comparing the nested discardable-attribute case with the top-level case. Read the SymbolUserAttrInterface verification path and the related walkSymbolRefs behavior, then add or update the negative test so nested symbol references produce the expected diagnostic.

Written by the indexing model from the issue text.

Assessment

Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.