Skip to content

Added mlir_opt property to Qjit #1579

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

Merged
merged 42 commits into from
Apr 3, 2025
Merged

Added mlir_opt property to Qjit #1579

merged 42 commits into from
Apr 3, 2025

Conversation

mehrdad2m
Copy link
Contributor

Context:
Currently, if one wants to inspect the intermediate IR, they should use keep_intermediate=True alongside get_compilation_stage. However, for custom passes or dialects lacking backend support, running the whole compilation pipeline (mlir-opt, mlir-trasnlate, llc) could fail hindering IR inspection. It would be nice if we have an easy way to disable mlir-trasnlate and llc from frontend since what we are interested to inspect is the transformed MLIR IR as opposed to the final binary.

Description of the Change:
Introduces a QJIT.mlir_opt property. This property provides access to the optimized MLIR IR after mlir-opt transformations, but before lowering to LLVM IR (skipping mlir-translate and llc). This differs from QJIT.mlir, which only displays the initial, unoptimized MLIR.

Benefits:
Simplifies inspection of optimized MLIR IR.

Possible Drawbacks:
None

Related GitHub Issues:

@mehrdad2m mehrdad2m requested review from dime10 and sengthai March 21, 2025 15:16
Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.48%. Comparing base (d696bbd) to head (84ecf97).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1579      +/-   ##
==========================================
+ Coverage   96.45%   96.48%   +0.02%     
==========================================
  Files          80       80              
  Lines        8584     8592       +8     
  Branches      827      828       +1     
==========================================
+ Hits         8280     8290      +10     
+ Misses        247      246       -1     
+ Partials       57       56       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@sengthai sengthai left a comment

Choose a reason for hiding this comment

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

Thanks for PR, @mehrdad2m! 🚀

@dime10
Copy link
Contributor

dime10 commented Mar 21, 2025

@mehrdad2m Are you rebasing this on Erick's changes? I think that would make more sense then modifying the shared lib class to be able to do nothing.

@mehrdad2m
Copy link
Contributor Author

@mehrdad2m Are you rebasing this on Erick's changes? I think that would make more sense then modifying the shared lib class to be able to do nothing.

well I agree that for the purpose mlir_opt we won't need to call the run_from_ir anymore but even in that branch, shouldn't we support a case where run_from_ir returns a None object? i.e when we call run_from_ir with lower_to_llvm=False

@dime10
Copy link
Contributor

dime10 commented Mar 21, 2025

@mehrdad2m Are you rebasing this on Erick's changes? I think that would make more sense then modifying the shared lib class to be able to do nothing.

well I agree that for the purpose mlir_opt we won't need to call the run_from_ir anymore but even in that branch, shouldn't we support a case where run_from_ir returns a None object? i.e when we call run_from_ir with lower_to_llvm=False

Fair, I guess that's the question if we still need run_from_ir to be able to return nothing (well run_from_ir is fine I think, the problem is afterwards instantiating a CompiledFunction instance, inserting it into the cache, etc). But let's assume we do still need this, then I think I would feel better if we never instantiated a CompiledFunction object (which instantiates the SharedLibraryManager) to begin with, than having an instance of it that does nothing. Maybe I'm overly cautious but it seems fraught with potential for bugs because we are breaking an assumption of the class, and there is uncertainty how it interacts with the other systems (e.g. the compilation cache).

@mehrdad2m
Copy link
Contributor Author

@mehrdad2m Are you rebasing this on Erick's changes? I think that would make more sense then modifying the shared lib class to be able to do nothing.

well I agree that for the purpose mlir_opt we won't need to call the run_from_ir anymore but even in that branch, shouldn't we support a case where run_from_ir returns a None object? i.e when we call run_from_ir with lower_to_llvm=False

Fair, I guess that's the question if we still need run_from_ir to be able to return nothing (well run_from_ir is fine I think, the problem is afterwards instantiating a CompiledFunction instance, inserting it into the cache, etc). But let's assume we do still need this, then I think I would feel better if we never instantiated a CompiledFunction object (which instantiates the SharedLibraryManager) to begin with, than having an instance of it that does nothing. Maybe I'm overly cautious but it seems fraught with potential for bugs because we are breaking an assumption of the class, and there is uncertainty how it interacts with the other systems (e.g. the compilation cache).

I see, that's a fair concern. I think now that with Erick's PR, we can call an _opt method, there is not point in allowing lower_to_llvm to be False inside run_from_ir. So we can assert lower_to_llvm to be true. This also agrees with the name of the method (run_from_ir) because we are not running anything if lower_to_llvm is False. I will change the target of this PR to be Erick's branch then.

@mehrdad2m mehrdad2m changed the base branch from main to eochoa/2025-03-07/opt-output March 21, 2025 19:41
Base automatically changed from eochoa/2025-03-07/opt-output to main April 1, 2025 13:40
@mehrdad2m mehrdad2m requested a review from erick-xanadu April 1, 2025 20:57
Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

I would prefer if we didn't add a lot more to the qjit object. But I do find it easier to just access this field to get the MLIR after optimizations. The other approach I see is by passing the output of circuit.mlir to quantum_opt. But I also don't like it too much. So 👍

@mehrdad2m mehrdad2m requested a review from dime10 April 2, 2025 18:46
Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Thanks Mehrdad 🎉

@mehrdad2m mehrdad2m merged commit a7c5e0b into main Apr 3, 2025
45 checks passed
@mehrdad2m mehrdad2m deleted the qjit-mlir-opt branch April 3, 2025 15:23
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.

4 participants