Skip to content

Conversation

@slyubomirsky
Copy link
Contributor

Implements proposal #14899 for TIR, by the same method as PR #15140. Namely, a TIR PrimFunc can be specified to be private (without a global symbol attribute) in TVMScript in the prim_func decorator. By default, PrimFuncs are not private, so they will have a global_symbol attribute that is mapped to their name.

Example usage:

# not private: its global symbol will be "func"
@T.prim_func
def func(...): ...

# no global symbol included
@T.prim_func(private=True)
def func(...): ...

This did require changing very, very many tests, unfortunately.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jun 27, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

1 similar comment
@tvm-bot
Copy link
Collaborator

tvm-bot commented Jun 27, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

from tvm.tir.schedule import Schedule, Trace


def assert_structural_equal_gs(f1: PrimFunc, f2: PrimFunc, *args: Any, **kwargs: Any) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the name but I didn't want it to be too verbose because this has to be used in a lot of places. Also, is there a better file where this should reside? It's used in tests outside of those related to scheduling.


def _check(original, transformed):
mod = tvm.IRModule.from_expr(original)
mod = tvm.IRModule.from_expr(original.with_attr("global_symbol", "main"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should from_expr include a global symbol by default? The only reason I hesitated on implementing that is because Relay and Relax also use from_expr and I didn't want to mess with that.

@slyubomirsky slyubomirsky requested review from junrushao and tqchen June 27, 2023 21:39
@slyubomirsky slyubomirsky force-pushed the privacy-annotation-tir branch from e528cb1 to dcbaff6 Compare June 28, 2023 19:26
@I.ir_module
class Expected:
@T.prim_func
@T.prim_func(private=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throughout these Relax tests, I assumed that most PrimFuncs we will add through fusion, etc, are likely not intended to be public. Please let me know if this assumption is mistaken.

Note that the below keys are reserved:
* "primfunc_name_hint" is reserved for passing a name hint
to the PrimFunc that gets generated.
* "primfunc_public" is reserved for indicating whether the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For emitting PrimFuncs from Relax, I assume private is the default we want. Happy to do it the other way if that's desirable.

@slyubomirsky
Copy link
Contributor Author

slyubomirsky commented Jun 29, 2023

If anyone has any ideas, I don't know what's wrong with the current failing GPU tests in the last CI. Nothing in them seems to have anything to do with the changes in this PR (in both cases, no global symbols were added or removed: all PrimFuncs either already didn't have them or already had them).

edit: One suggestion was that it had to do with the SplitHostDevice changes, we'll see if that works out.

edit 2: That was right! Thank you, @csullivan

@slyubomirsky slyubomirsky force-pushed the privacy-annotation-tir branch from 78dae74 to 5693c75 Compare June 30, 2023 15:36
@tqchen
Copy link
Member

tqchen commented Jul 1, 2023

Thank you @slyubomirsky , do you mind sending this PR to main instead? we can then merge it back to unity

@slyubomirsky
Copy link
Contributor Author

Okay. I'll have to get rid of the Relax test changes to have it in main.

@slyubomirsky
Copy link
Contributor Author

slyubomirsky commented Jul 3, 2023

Reposted for mainline: #15214. There seem to be some different tests in mainline, so I'll probably have to chase after some new failures.

junrushao pushed a commit that referenced this pull request Jul 20, 2023
Implements proposal #14899 for TIR. Adapted from Unity PR #15171, by the same method as PR #15140. Namely, a TIR PrimFunc can be specified to be private (without a global symbol attribute) in TVMScript in the `prim_func` decorator. By default, `PrimFunc`s are not private, so they will have a `global_symbol` attribute that is mapped to their name.

Example usage:
```python
# not private: its global symbol will be "func"
@T.prim_func
def func(...): ...

# no global symbol included
@T.prim_func(private=True)
def func(...): ...
```

This did require changing very, very many tests, unfortunately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants