[Mlir-commits] [mlir] [mlir][transform] Fix handling of transitive include in interpreter. (PR #67560)

[Ingo Müller]

ingomueller-net wrote:

@ftynse: This is a new attempt at #67241 along the lines of your suggestions. Please take a look. It is worth mentioning that (i) I merged the stages (1) and (2) from your suggestion, and (ii) did not implement the optimization of only including needed symbols.

I have identified one more problem that may require a major change: it is currently not always possible to run the interpreter pass several times. The problem is the following: if the script contains a declaration `@foo` and the library contains a matching definition, then than definition is merge into the transform module upon its first invocation. In the second invocation, however, `@foo` is now a *definition* in the transform module, so merging in the library module leads to a `doubly defined symbol` error. This did not happen before because only the (directly) missing symbols were merged, and `@foo` is not missing after the first invocation anymore.

I see two ways out: (A) only merge in missing symbols (plus their dependencies) as previously and make all imported symbols private, or (B) separate the merging from the interpreter into a separate pass. I have the feeling that Approach (A) would make the merging again more special to the interpreter and possibly somewhat brittle and Approach (B) would be more generic and possibly more performant. (If the interpreter pass is really used several times, linking has to be done each time, while with a dedicated linking pass, it would only be done once.)

I imagine the linking pass from Approach (B) to be called something like `load-transform-dialect-interpreter-module` or similar, take the `transform-library-file-name` and `transform-library-file-name` arguments (as well as their extensions planned in #67120), and then only do the loading and merging/linking (but not interpret the result). The main interpreter pass would then simply run the interpreter on the input module and not bother with any loading or merging. Also, I think I'd implement that separation *first* and then rebase this change on top of it.

What do you think?

https://github.com/llvm/llvm-project/pull/67560