Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REFACTOR][IR] Migrate Pass/PassContext ObjectRef to not-null #5717

Closed
wants to merge 4 commits into from

Conversation

ANSHUMAN87
Copy link
Contributor

Refer #5318

@tqchen, @jroesch : Please help review, Thanks!

@ANSHUMAN87 ANSHUMAN87 force-pushed the ref-object-refactor-pass branch from be3596a to 6b3dd52 Compare June 3, 2020 06:57
include/tvm/ir/transform.h Show resolved Hide resolved
@@ -689,7 +689,7 @@ class CompileEngineImpl : public CompileEngineNode {
cache_node->funcs = (*f)(cfunc->schedule, all_args, cache_node->func_name, key->source_func);
} else {
using tvm::transform::PassContext;
With<PassContext> fresh_pass_ctx_scope(PassContext::Create());
With<PassContext> fresh_pass_ctx_scope(static_cast<PassContext>(PassContext()));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think static cast would be necessary here, With fresh_pass_ctx_scope(PassContext()); should suffice once we defined the copy constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tqchen : Thanks! I tried with copy constructor using, but it did not work out.

Copy link
Member

Choose a reason for hiding this comment

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

Would be interesting to see what error message you got. As copy constructor is clearly defined in there.
We can also use With<PassContext> fresh_pass_ctx_scope();, which invokes the default constructor(in this case the new pass context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tqchen : Sorry for late reply! If i dont use static_cast the testcase fails with additional assert count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tqchen : I have changed it to default constructor now, please check. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tqchen : I have changed it to default constructor now, please check. Thanks!

@tqchen : Sorry! This change does not work. I made a mistake in code build, cause of that it got passed last time! So i have to use static_cast for the test case to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tqchen : Any thoughts on this comment?

@ANSHUMAN87 ANSHUMAN87 force-pushed the ref-object-refactor-pass branch 2 times, most recently from b21369b to 212a68c Compare June 8, 2020 16:48
@tqchen
Copy link
Member

tqchen commented Aug 20, 2020

close for now and let us revisit later. Thanks @ANSHUMAN87

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