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]

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.56%. Comparing base (d83fb10) to head (68e29eb).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2244   +/-   ##
=======================================
  Coverage   97.56%   97.56%           
=======================================
  Files          93       93           
  Lines       11210    11210           
  Branches     1068     1068           
=======================================
  Hits        10937    10937           
  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]

Nice work Paul!

[dime10]

Usually I jump over these, but I think typically you'd want more context in the release notes. Why is this relevant, where would a user or developer have encountered this, what is the implications, etc. Doesn't have to be long (succinct is good!), but it's more important to highlight the context over the thing that changed.

[paul0403]

8eaf2a5 added some context here

[dime10]

This is just me being paranoid, but since we've had so many incidents with object lifetimes I want to confirm with you that the string storage lives for as long as this set does?

[paul0403]

This is just the struct's internal handle to the set (hence the pointer), I created this set outside the struct in the global "main" function of the pass where the pattern struct is called:

        llvm::SmallSet<StringRef, 8> externalFuncDeclNames;
        renameFunctions.add<RenameFunctionsPattern>(context, &symbolTables, &externalFuncDeclNames);

        // ...stuff...
        inlineNested.add<InlineNestedModule>(context, externalFuncDeclNames,
                                             &alreadyInlinedFuncDeclNames);

I need the rename pattern to record which names are external func decls, so the subsequent inline pattern knows to handle them specially. Note that the rename pattern needs to write to the set so it's passed in as a pointer, but the inline pattern only needs to read it so it's passed in as const reference.

So the global llvm::SmallSet<StringRef, 8> externalFuncDeclNames is the one that controls the lifetime. And there is only ever one set! The local variables are just handles, aliasing the global one.

[dime10]

Interesting, didn't even know this mechanism existed. Are you comfortable breaking the constness of the mlir rewrite function?

[paul0403]

I guess all I needed was a set to record some names. I can create this set outside the struct and pass it in as reference 👍

[paul0403]

ee3b2f7

[dime10]

Is op a module? I'm not sure we actually need to walk it recursively because I've never seen function definitions being nested in other functions 🤔 I simple iteration might suffice

[paul0403]

op is the qnode module. Yeah come to think of it direct iteration makes more sense, this is also what Erick chose in other places in this pass as well.

[paul0403]

5ef407d