Do not rename external func decls during --inline-nested-modules

[paul0403]

Context:
One of the first steps in the compilation pipeline is --inline-nested-modules. This pass lifts the per-qnode module up into the global module of the qjit.

Because (usually) the symbol table of a module is isolated from above, multiple modules in parallel may contain the same names inside. Therefore when inlining child modules into the global module, we need to rename them to unique names.

However, there's a small problem. Each qnode module has their own specific transform sequence, so the quantum passes (applied during --apply-transform-sequence) have to be resolved before --inline-nested-module. If a pass generates a function declaration to an external API (e.g. runtime functions), that name must not be altered.

Description of the Change:
Do not perform renaming on external function declarations from within the qnode modules during --inline-nested-modules.
Only inline the first occurrence of such func decls from the qnode modules into the global qjit module.

Benefits:
Quantum passes that generate calls to other APIs can work.
The flipside is also true! API developers do not have to maintain multiple aliases for their API functions (e.g. gridsynth pass #2140 )

[sc-105020]

[github-actions]

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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.55%. Comparing base (a5aa5ec) to head (b792d8d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2244   +/-   ##
=======================================
  Coverage   97.55%   97.55%           
=======================================
  Files          93       93           
  Lines       11180    11180           
  Branches     1065     1065           
=======================================
  Hits        10907    10907           
  Misses        208      208           
  Partials       65       65           

☔ 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.

[josephleekl]

Thanks @paul0403 ! This makes sense to me.

I don't know enough about the other passes in Catalyst, will any of them be affected by this? e.g. decompose lowering?

[dime10]

Wow there is a lot of invocations of the greedy pattern applicator in this pass. I think this may indicate a misuse of the infrastructure.
Patterns are meant to be small rewrites that are independent, composable, and repeatable (typically).
If this is not the case for a pass, and instead we want to apply a specific sequence of rewrites one time, then we should do so directly which will be much more efficient.

I'm not saying this is 100% the case here, but it definitely looks suspicious.

[dime10]

Thanks for catching this Paul!

Approach seems ok, although perhaps a bit hacky. Have you considered wether it's possible to directly deduplicate the external functions while we're inlining the module? (instead of rename -> restore)

[paul0403]

I tried this, but it was a bit difficult...

The main issue is how Erick structured this pass into multiple independent stages, each with its own pattern (aka your other comment above). One of the stages/patterns is responsible for the renaming. My very first commit on this PR is actually just skipping the renaming for external function declarations.

But then, one of the next stages/patterns updates the call ops to call those newly name-uniqued declarations. This would now involve passing some helper containers across different rewrite pattern struct! Because I would need the renaming pattern struct to record a list of external names, so that the call update pattern can know to leave those untouched.

This might be a problem inside the pass code, so it's not that big of a dealbreaker. But there's actually something more serious at the IR level. Consider this IR (which is the structure that will be added by Gridsynth pass on a qjitted module with two qnodes)

module @global {

  module @local0 {
    func.func private @f()
    func.func @main() {
      func.call @f() : () -> ()
      return
    }
  }

  module @local1 {
    func.func private @f()
    func.func @main() {
      func.call @f() : () -> ()
      return
    }
  }

}

When inlining the two f declarations, if we do not rename them to f_0 and f_1, then the inlining itself will fail! The inlining of the second f will say it clashes with an existing f in the parent module (result of inlining the first f).

So what about not inling the second f, but just discard it? Well, that discard can't even happen, because regions are isolated from above, so if we try to discard it, its call in its module will say it is calling an undefined symbol! So we have to inline that second call first (no renaming required: the callee and the inlined first decl are both f in this approach). But now this would start to involve big refactoring of Erick's original structure, since now we are inling call ops at the renaming stage.

I very much agree that this structure is quite weird. This might actually be one of these situations where a raw runOnOperation() with raw logic is more suited then rewrite patterns. But I don't want to spend too much time refactoring, since

• (a) gridsynth is hard-blocked by this (unless we are good with attaching _0s to all of the runtime solver function names, which I think obviously we shouldn't)
• (b) the new quantum scope abstraction proposal might make this entire pass obsolete anyway

[dime10]

Thank you for the in-depth analysis!

So what about not inling the second f, but just discard it? Well, that discard can't even happen, because regions are isolated from above, so if we try to discard it, its call in its module will say it is calling an undefined symbol!

This I don't quite follow. If f already exists in the scope we are inlining into (from inlining the first module), why do we have to inline the second f in order to inline the second module? I would imagine that it's kind of like our getOrCreateFunction helpers, where we would only inline if it doesn't already exist. Unless we don't have granular control over the inlining, i.e. we are not doing the inlining ourself but have to do the entire module at once.

so if we try to discard it, its call in its module will say it is calling an undefined symbol!

By discard you mean deleting it before we inline? Generally we should be allowed to create invalid IR as an intermediate step (in fact I don't think you can do a lot of meaningful rewrites without this), and the only requirement is that the IR is legal after a pass is run no?

But I don't want to spend too much time refactoring

Absolutely, if this is too complicated then I'm happy to proceed with the do-undo approach :)

[paul0403]

I got it to work, now we don't have to rename and undo rename via a discardable in-between attribute 🥳 d5b7c32