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

Update mlir version to have closure-aware backward slice #1585

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

Conversation

paul0403
Copy link
Member

@paul0403 paul0403 commented Mar 24, 2025

Context:
Proposal: we update the mlir commit we track to see llvm/llvm-project#114452

When using backward slice to track def-use chains, sometimes there are closure variables not explicitly visible as an operand, e.g.

    %7 = quantum.insert %5[%2], %out_qubits : !quantum.reg, !quantum.bit
    %8 = scf.if %1 -> (!quantum.reg) {
      %15 = quantum.extract %7[ 0] : !quantum.reg -> !quantum.bit
      %out_qubits_5 = quantum.custom "RZ"(%cst_1) %15 : !quantum.bit
      %16 = quantum.insert %7[ 0], %out_qubits_5 : !quantum.reg, !quantum.bit
      scf.yield %16 : !quantum.reg
    } else {
      scf.yield %7 : !quantum.reg
    }
    %9 = quantum.extract %8[%0] : !quantum.reg -> !quantum.bit
    %10 = quantum.extract %8[ 1] : !quantum.reg -> !quantum.bit
    %out_qubits_4:2 = quantum.custom "CNOT"() %9, %10 : !quantum.bit, !quantum.bit
    %11 = quantum.insert %8[%0], %out_qubits_4#0 : !quantum.reg, !quantum.bit

when running backward slice on %11 to track all necessary previous values that dominate %11, the current mlir commit we track cannot detect %7, because %7 is not an explicit operand of the scf.if op, but implicitly captured by region closure. The proposed commit to track adds a new option, omitUsesFromAbove , to the mlir backward slice analysis, and can be turned off to include clousure variables.

Description of the Change:
Update the mlir commit we track

Benefits:
New mlir features available

Possible Drawbacks:
Updating versions might break things

@paul0403 paul0403 requested a review from a team March 24, 2025 14:58
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@dime10
Copy link
Contributor

dime10 commented Mar 24, 2025

Have you tested locally? Not sure where exactly this commit is, but MLIR deprecated the old bufferization functions which we were working on replacing to be able to update.

Also, normally the llvm commit is matched exactly to the JAX dependency.

@paul0403
Copy link
Member Author

Have you tested locally? Not sure where exactly this commit is, but MLIR deprecated the old bufferization functions which we were working on replacing to be able to update.

Also, normally the llvm commit is matched exactly to the JAX dependency.

I haven't tried locally, I wanted to use CI to see what happens. I am aware of the two points you mentioned here so I am not super in favor of this update either, but I just wanted to see how much work would be necessary if we did want to update, comparing with writing the described functionality ourselves.

@paul0403
Copy link
Member Author

Ok I think the update might be too much, we can just write this simple functionality ourselves for this one purpose :)

Although on the other hand, an argument can be made that we should update dependencies more often to catch errors early?

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

Successfully merging this pull request may close these issues.

2 participants