Skip to content

Conversation

@MasterJH5574
Copy link
Contributor

The first version of specialize() introduced in PR #8354 didn't update the attrs of PrimFuncs. This might lead to something unexpected when a function attribute is a PrimExpr which contains the variables being specialized.

So this PR fixes this issue, and meanwhile provides a regression test. The regression test is a naive illustration of the described issue.

(Besides, this PR also fixes some typos.)

cc @Hzfengsy @tqchen @comaniac @jcf94

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

LGTM

@tqchen
Copy link
Member

tqchen commented Jul 30, 2021

It is anti-pattern to have attrs that refers to the body of a function(because there is no guarantee that every pass will update the corresponding attrs). So we should not support this kind of update. Instead, we should have well-form check if report an error if such pattern happens.

@junrushao
Copy link
Member

@tqchen a bit of context here: we need to support manually specialized flop counter for BSR, and @vinx13’s solution is to add a function attributed that can be specialized. From my PoV it might be the right approach

@MasterJH5574
Copy link
Contributor Author

Currently only meta-schedule needs this feature of specialize. If we cannot decide whether to support it now, perhaps we can support it in TIR sub-branch, and think of other potential ways to solve the flop counter issue in the future. What do you guys think? @junrushao1994 @tqchen

@tqchen
Copy link
Member

tqchen commented Jul 31, 2021

We should find a better way to implement the feature(e.g. flop counting) without relying on this pattern. Additionally, it would be great if we can add a well-form checker to ensure that things are in good form and reject programs whose attrs refers to the function body.

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