Skip to content

Conversation

@Lunderberg
Copy link
Contributor

Prior to this commit, ApplyPassToFunction removed functions from the IRModule to hide them from the inner ir.transform.Pass. The dangling GlobalVar references to those functions meant that the intermediate IRModule was ill-formed This commit updates the ApplyPassToFunction utility to instead replace the functions with ExternFunc nodes. This still prevents the inner ir.transform.Pass from having visibility into functions that should not be mutated, but provides a well-formed IRModule.

@Lunderberg
Copy link
Contributor Author

A follow-up commit to #16801

@slyubomirsky
Copy link
Contributor

Are the substituted global vars restored afterwards? I couldn't find a place where they were put back. I would be worried about these extern funcs ending up in the final module.

@Lunderberg
Copy link
Contributor Author

The GlobalVar instances themselves don't need to be changed, only the functions to which they point. This is why the pre-transform steps don't need to modify any function bodies, because the GlobalVar is still present in the IRModule. After running the transform, the values from new_subset are only copied back to the original mod if they weren't one of the dummy ExternFunc instances.

@Lunderberg
Copy link
Contributor Author

Lunderberg commented Apr 4, 2024

The CI failure here will be resolved with #16843.

Edit: #16844 is the commit that will resolve the CI failure here.

@slyubomirsky
Copy link
Contributor

Wait, a global var can be mapped to an ExternFunc? That's wild.

Copy link
Contributor

@slyubomirsky slyubomirsky left a comment

Choose a reason for hiding this comment

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

I had no idea that it was possible to have an ExternFunc as a module entry. If this is behavior used elsewhere, I should probably mention this in the specification. Otherwise, this "should" be safe because most accesses of global vars have to check if the value is a Relax Function since it could be a PrimFunc... then again, there might be hazards if a pass assumes (reasonable) that those are the only two possibilities. In any case, this is an improvement compared to passing a potentially malformed module altogether.

@Lunderberg
Copy link
Contributor Author

Yup, relax::ExternFunc is a subclass of BaseFunc, and can be stored directly in an IRModule. At some point, I want to use this to specify when the lookup of a PackedFunc in the global registry occurs. Currently, there's a discrepency between Relax and TIR, where Relax performs the lookup when the VirtualMachine is initialized, while TIR performs the lookup just before the call is performed. I'd like to make these more similar, with an ExternFunc being resolved either at initialization (declared in IRModule), at the start of a function (ExternFunc bound to a variable), or at the callsite (ExternFunc used inline).

But the main impetus for that would have been the LazyTransformParams pass, and with the LazyGetInput accepting a callback parameter, there isn't as much need for it.

Prior to this commit, `ApplyPassToFunction` removed functions from the
`IRModule` to hide them from the inner `ir.transform.Pass`.  The
dangling `GlobalVar` references to those functions meant that the
intermediate `IRModule` was ill-formed This commit updates the
`ApplyPassToFunction` utility to instead replace the functions with
`ExternFunc` nodes.  This still prevents the inner `ir.transform.Pass`
from having visibility into functions that should not be mutated, but
provides a well-formed `IRModule`.
@Lunderberg Lunderberg force-pushed the relax_well_formed_apply_pass_to_function branch from ea7ad63 to 5ce7957 Compare April 5, 2024 12:23
@Lunderberg
Copy link
Contributor Author

Rebased this PR onto main, since #16844 has now landed.

@Lunderberg Lunderberg merged commit 9b5a7a4 into apache:main Apr 5, 2024
@Lunderberg Lunderberg deleted the relax_well_formed_apply_pass_to_function branch April 5, 2024 18:21
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.

2 participants