-
Notifications
You must be signed in to change notification settings - Fork 57
Issue #1714: Add variable names to the IR #2054
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
base: main
Are you sure you want to change the base?
Issue #1714: Add variable names to the IR #2054
Conversation
Add test_alice_and_bob_parameters. Rename test_alice to test_alice_qubit. Rename test_alice_and_bob to test_alice_and_bob_qubits.
lower_jaxpr_to_mlir: add arg_names, and pass them on to jaxpr_to_mlir. jaxpr_to_mlir: change parameter order, add arg_names, and pass them on to custom_lower_jaxpr_to_module. custom_lower_jaxpr_to_module: add arg_names, and pass them on to lower_jaxpr_to_fun. This latter function builds arg_locs with the arg_names and appends them to the function's entry_block. qjit: add use_nameloc parameter. CompileOptions: add use_nameloc option. _options_to_cli_flags: add '-mlir-use-nameloc-as-prefix' if use_nameloc=True. test_debug.py: add test_option_use_nameloc.Add frontend/test/lit/test_option_use_nameloc.py. TODO: the name location information is added to the MLIR module, but still not consumed by quantum-opt. TODO: should we try adding name location information for other than funcion arguments?
… name location information in Catalyst Add QJIT.get_arg_names() to construct a list of argument names for a qjit function. Change QJIT.mlir and QJIT.mlir_opt to optionally use an MLIR string with debug info. Change canonicalize function to optionally pass --use-nameloc-as-prefix. Add --use-nameloc-as-prefix to Catalyst CLI options. Change QuantumDriverMain to optionally use useNameLocAsPrefix printing flag when printing the MLIR module. Add test_get_arg_names.py. Change test_option_use_nameloc.py to check that: 1) MLIR module contains location information. 2) MLIR code contains location information. 3) f.mlir uses the qjit function parameter names, %x and %y, instead of %arg0 and %arg1.
Thanks @rturrado for the submission. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2054 +/- ##
==========================================
+ Coverage 97.49% 97.50% +0.01%
==========================================
Files 91 91
Lines 10604 10624 +20
Branches 995 1001 +6
==========================================
+ Hits 10338 10359 +21
+ Misses 211 210 -1
Partials 55 55 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thank you for patience Roberto, I should be able to come back to this PR in the next few days :) |
Add print_mlir_opt to frontend/test/lit/utils.py. Invoke print_mlir_opt from frontend/test/lit/test_option_use_nameloc.py.
No rush at all, @dime10 ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, another excellent PR! 🥇
``--use-nameloc-as-prefix[=<true|false>]`` | ||
"""""""""""""""""""""""""""""""""""""""""" | ||
|
||
Print SSA IDs using their name location, if provided, as prefix. By default, name location information is not used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could say a bit more about what a "name location" is :)
assert str(f.mlir_module.body.operations[0].arguments[1].location) == 'loc("y")' | ||
|
||
print_mlir(f, 0.3, 0.4) | ||
print_mlir_opt(f, 0.3, 0.4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the second line do anything here? Typically we the print the IR in the lit tests so that the CHECK statements can be matched against it, but if they already matched once, they aren't going to be matched against anything further down in the stream.
@@ -0,0 +1,31 @@ | |||
// RUN: quantum-opt %s -mlir-use-nameloc-as-prefix -split-input-file | FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not our pass is it? If not, we're just testing upstream code right 🤔
if self.compile_options.use_nameloc: | ||
stdin = self.mlir_module.operation.get_asm(enable_debug_info=True) | ||
else: | ||
stdin = str(self.mlir_module) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this different from using this method unconditionally?
if self.compile_options.use_nameloc: | |
stdin = self.mlir_module.operation.get_asm(enable_debug_info=True) | |
else: | |
stdin = str(self.mlir_module) | |
stdin = self.mlir_module.operation.get_asm(enable_debug_info=self.compile_options.use_nameloc) |
|
||
return jaxpr, out_type, treedef, dynamic_sig | ||
|
||
def get_arg_names(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant to expose another method in the QJIT class as we want to keep it relatively stable and avoid class bloat. Could this be implemented as a free-standing function in the tracing
submodule (or other jax-related modules if more appropriate, but I think type_signatures.py
looks pretty relevant)?
Context:
We can investigate the option of passing down source code information like variable names from the frontend down to the generated ir, using MLIR's location mechanism. The additional info will make generated code more readable.
Description of the Change:
User -> Frontend:
@qjit(use_nameloc=True)
to use name location information in the generated MLIR code.Frontend -> Catalyst CLI: whenever
use_nameloc
is enabled:stdin=self.mlir_module.operation.get_asm(enable_debug_info=True)
instead ofstdin=str(self.mlir_module)
.--use-nameloc-as-prefix
inoptions
.Catalyst CLI -> frontend: whenever
--use-nameloc-as-prefix
is enabled:useNameLocAsPrefix
printing flag.mlirModule->print(oss, opPrintingFlags)
instead ofoss << *mlirModule
.Benefits:
@qjit(use_nameloc=True)
is an eay way for the user to specify that they want to use name location information.--use-nameloc-as-prefix
option.Possible Drawbacks:
get_arg_names
function may not be robust enough.%x
instead of%arg0
), but maybe not so nicely for results of equations (e.g.,%jit28ok292Fjit28jit_f292Fmul
instead of%0
). An option would be to add name location information only for function parameters.Related GitHub Issues:
#1714