[MLIR] Attribute caching logic during parsing causes issues for custom assembly formats

[knalbant]

(#151267) Fix duplicated attribute nodes in MLIR bytecode deserialization has caused issues with parsing of typed attributes in one of our internal dialects at Modular.

We have IR with TypedAttrs that looks like:

example.param.load : !example.data<0> = <#example.value>
example.param.load : !example.data<1> = <#example.value>

And this logic (LINK) to check if an attribute is a TypedAttr based on whether the value is followed by a colon is proving to be problematic.

[llvmbot]

@llvm/issue-subscribers-mlir

Author: Kenan (knalbant)

[(#151267) Fix duplicated attribute nodes in MLIR bytecode deserialization](https://github.com//pull/151267) has caused issues with parsing of typed attributes in one of our internal dialects at Modular.

We have IR with TypedAttrs that looks like:

example.param.load : !example.data<0> = <#example.value>
example.param.load : !example.data<1> = <#example.value>

And this logic (LINK) to check if an attribute is a TypedAttr based on whether the value is followed by a colon is proving to be problematic.

[knalbant]

@hankluo6

[weiweichen]

@joker-eph, wondering what's your take on this issue?

[Dinistro]

There seem to be multiple issues, see #155210

I'll thus revert the underlying change: #155214

[hankluo6]

Sorry for the late reply. The original design has issues when parsing types. I’ll look into it again.

[joker-eph]

@knalbant : could you add some coverage for this case with a test upstream?

[joker-eph]

@knalbant ping on this?

[knalbant]

We'll work on creating some tests here

[VerdagonModular]

I'm taking over for Kenan on that, though it might take a couple days, not used to the LLVM codebase yet.

[VerdagonModular]

This test passes on main and fails with the (now reverted) PR:

// RUN: mlir-opt %s | mlir-opt | FileCheck %s

func.func @minimal_typed_attr_bug() {
  // CHECK: test.typed_attr i32 = #test.attr_with_self_type_param
  test.typed_attr i32 = #test.attr_with_self_type_param
  
  // Note the difference between i32 above and f32 below
  // CHECK: test.typed_attr f32 = #test.attr_with_self_type_param  
  test.typed_attr f32 = #test.attr_with_self_type_param
  
  return
}

Hope that helps. Would you like me to open a PR to add this somewhere?

[joker-eph]

Yes please if you can open a PR adding this test that would be great, thanks a lot!