Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve attribute during secret.generic lowering #1430

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ZenithalHourlyRate
Copy link
Collaborator

Fixes #1426

  • secret-distribute-generic preserves the dialect attrs from secret.generic op argument to the func.func op argument
  • secret-to-<scheme> preserves the dialect attrs from secret.generic's inner op to converted op
  • <scheme>-to-lwe preserves the attribute originally
  • lwe-add-debug-port will transplant the attr to the CallOp, for debugging purpose, see Pass Attribute to debug function call #1422
  • lwe-to-<backend> will preserve the attr in CallOp

Conceptually, this PR suggests that dialect attribute should be preserved and managed by the programmer (of corresponding dialect) while other attribute has no guarantee on how long it will live.

// special treatment for memref.alloc, which has alignment attr
// that should be preserved
// TODO: secret-to-cggi should handle this instead of here
for (auto &namedAttr : innerOp.getAttrs()) {
Copy link
Collaborator

@j2kun j2kun Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we actually (or also?) want to preserve the attributes on the secret.generic op? That's where the mgmt.mgmt attributes are set during the lowering, no?

I bring this up because I was working on a refactor of the context-aware type conversion in #1428 and I noticed this was happening, and the patterns were not able to find the mgmt attribute when only looking at the inner op.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I did a special treatment for mgmt.mgmt

  • First it is annotated on the inner op: %0 = arith.add {mgmt}
  • in secret-distribute-generic, the mgmt attr is moved to secret.generic, so %0 = secret.generic {mgmt} { arith.add }
  • context aware type converter for %0 then will look up the mgmt attr in secret.generic as now it becomes %0's defining op so there is no inner op involved

So it is secret-distribute-generic's responsibility to maintain secret.generic's attribute. Generally there are no other attribute in secret.generic as only secret-distribute-generic will create these ops (well for the wrap-generic created secret.generic its attribute should be cleaned, represented by moveDialectAttrsToFuncArgument here). All meaningful other attribute are annotated in the inner op.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think I misspoke. I was running into a situation like this:

%0 = secret.generic {mgmt1}
  ...
%1 = secret.generic (%0) {mgmt2}

When the lowering converts the first generic to a ckks op, it doesn't currently preserve any attributes. So when the second generic is processed, it looks for the mgmt attr of the replacement for %0, which had no attribute at all.

In my PR I originally tried copying over just the inner op's attrs to the new ckks op, but the mgmt attr was not present on the inner op, just the enclosing generic.

I'm actually still confused about how the current code was able to find the mgmt attr in this situation, given that it should not have had access to the original op that defined %0 after the replacement was made.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the lowering converts the first generic to a ckks op, it doesn't currently preserve any attributes.

I intentionally made so. In old version of my PR I did preserve the mgmt attribute from first secret.generic to a ckks op and expect the second secret.generic to find the mgmt attribute there, but after inspecting OpAdaptor during conversion I find the following fun fact about DialectConversion:

During DialectConversion the old op and new op will both be present and the uses of the old op, while asked rewriter to replaceUses, is not committed until the last step (which is called unlink internally in DialectConversion). That is why all op modificaiton in DialectConversion has to happen through the rewriter. So for the second op, it will find the following IR

%0 = secret.generic {mgmt1}
%0_new = ckks.op /* no_attr */
  ...
// %0 has not been replaced by %0_new in %1's view!
%1 = secret.generic (%0) {mgmt2}

This is not undefined behavior but intended behavior of DialectConversion in MLIR, because ideally op conversion should not affect each other and the view of each ConvertOpRewritePattern should be the original IR instead of the intermediate IR, so it is guaranteed for the second op to find the original first op.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support preserving attributes in various lowering
2 participants