google / heir

A compiler for homomorphic encryption
https://heir.dev/
Apache License 2.0
231 stars 33 forks source link

segfault in heir-translate #727

Open j2kun opened 1 month ago

j2kun commented 1 month ago

Repro:

bazel run //tools:heir-opt -- --mlir-to-openfhe-bgv='entry-function=dot_product ciphertext-degree=8' $PWD/tests/openfhe/end_to_end/dot_product_8.mlir > $PWD/out.mlir
bazel run //tools:heir-translate -- --emit-openfhe-pke-header $PWD/out.mlir > out.h
j2kun commented 1 month ago

Error:

LLVM ERROR: can't create Attribute 'mlir::polynomial::IntPolynomialAttr' because storage uniquer isn't initialized: the dialect was likely not loaded, or the attribute wasn't added with addAttributes<...>() in the Dialect::initialize() method.

j2kun commented 1 month ago

From my investigation in discord:

ok I figured out the problem. The parser requires me to have a namespaced attribute or else it will fail to load the dialect
[3:17 PM]j2kun: this is weird. If I have an attribute like this: ring = #polynomial.ring<coefficientType = i32, coefficientModulus = 463187969 : i32, polynomialModulus = #polynomial.int_polynomial<1 + x**8>> it works, but if I have ring = #polynomial.ring<coefficientType = i32, coefficientModulus = 463187969 : i32, polynomialModulus = <1 + x**8>> then the parser doesn't load the attribute
[3:17 PM]j2kun: This is due to this check here: https://github.com/llvm/llvm-project/blob/19bbbcbedcbddcfc39202d7d801508a20baf83b6/mlir/lib/AsmParser/AttributeParser.cpp#L321-L323
[3:18 PM]j2kun: and this happens because the int_polynomial attribute omits the namespaced prefix in the default printer https://github.com/llvm/llvm-project/blob/19bbbcbedcbddcfc39202d7d801508a20baf83b6/mlir/lib/Dialect/Polynomial/IR/PolynomialAttributes.cpp#L22
[3:18 PM]j2kun: Any advice here?
asraa commented 1 month ago

Ugh, how weird. I was curious too and added

  mlir::DialectRegistry registry;
  registry.insert<mlir::polynomial::PolynomialDialect>();

before the mlir translate main, and even then it didn't fix the segfault. i assumed that even if the passes lazy loaded, that this would work. but maybe i don't understand what it means to register with the context per this link https://mlir.llvm.org/getting_started/Faq/#registered-loaded-dependent-whats-up-with-dialects-management

j2kun commented 1 month ago

Alex Zinenko on Discord:

This does look like a bug in the parser to me. If a dialect is registered, the parser should be able to load it on-demand. One shouldn't need to preload the dialect for parsing only.

j2kun commented 1 month ago

I can work on a fix over the next few days, but in the mean time we can disable the tests

asraa commented 1 month ago

This fixes it, but I don't know if it's "right" https://discord.com/channels/636084430946959380/642426447167881246/1248730040145809581

(I'm not sure there's other places that may need this too?)

j2kun commented 1 month ago

I think that would work but it will be bad for their performance concerns. Probably best to force it to load at parse time, instead of requiring the namespace be present.

On Fri, Jun 7, 2024, 1:08 PM asraa @.***> wrote:

This fixes it, but I don't know if it's "right" https://discord.com/channels/636084430946959380/642426447167881246/1248730040145809581

(I'm not sure there's other places that may need this too?)

— Reply to this email directly, view it on GitHub https://github.com/google/heir/issues/727#issuecomment-2155478031, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAS2PKV5OPZRCK2WOEUBF5DZGIHLBAVCNFSM6AAAAABI5OGF6WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGQ3TQMBTGE . You are receiving this because you were assigned.Message ID: @.***>

j2kun commented 4 weeks ago

Cf. repro upstream in https://github.com/llvm/llvm-project/pull/94838

j2kun commented 2 weeks ago

This was fixed upstream in https://github.com/llvm/llvm-project/pull/96242