Skip to content

Conversation

@Lunderberg
Copy link
Contributor

In rare cases, a PrimFunc may be annotated with StructInfo, to indicate that it is an impure function with specific shapes for the parameters. If struct info is present, it is invalidated when specializing a PrimFunc, and should be cleared.

In rare cases, a `PrimFunc` may be annotated with `StructInfo`, to
indicate that it is an impure function with specific shapes for the
parameters.  If struct info is present, it is invalidated when
specializing a `PrimFunc`, and should be cleared.
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.

The change seems straightforward but I would be more curious to hear about why it's necessary (i.e., what problems were being caused) and when the StructInfo is being set for PrimFuncs.

Comment on lines +332 to +333
While a PrimFunc usually doesn't have a `relax.StructInfo`, the
field can be populated in some edge cases. If that PrimFunc is
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know of times when we're doing it? It should be something we do consistently or not at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a couple of MLC (example) where the a dynamic PrimFunc is annotated with the relax.FuncStructInfo. This allows the R.call_tir shape inference to work correctly, even after the arguments are mutated.

IIRC, the primary reasons not to add FuncStructInfo to PrimFuncs was that primfuncs didn't follow the relax requirements for functional purity. Since we now have the purity expressed within FuncStructInfo, I think that means we could add FuncStructInfo to every PrimFunc on construction, without making any incorrect statements about the functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Always specifying it sounds like a better approach, as long as it would not interfere with anything in TIR.

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.

This change is fine but we should ultimately be consistent about whether or not to include StructInfo on PrimFuncs. Per the above discussion, always including it might cause fewer headaches.

@tqchen tqchen merged commit 40dd376 into apache:main Mar 9, 2024
@Lunderberg Lunderberg deleted the primfunc_remove_sinfo_in_specialize branch March 10, 2024 17:20
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Mar 12, 2024
In rare cases, a `PrimFunc` may be annotated with `StructInfo`, to
indicate that it is an impure function with specific shapes for the
parameters.  If struct info is present, it is invalidated when
specializing a `PrimFunc`, and should be cleared.
thaisacs pushed a commit to thaisacs/tvm that referenced this pull request Apr 3, 2024
In rare cases, a `PrimFunc` may be annotated with `StructInfo`, to
indicate that it is an impure function with specific shapes for the
parameters.  If struct info is present, it is invalidated when
specializing a `PrimFunc`, and should be cleared.
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.

3 participants