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

Add support for absolute opeartion #1406

Merged
merged 3 commits into from
Jul 13, 2018

Conversation

PariksheetPinjari909
Copy link
Contributor

@PariksheetPinjari909 PariksheetPinjari909 commented Jul 9, 2018

This PR adds support for absolute operation.

y : Expr
The result.
"""
return call_pure_intrin(x.dtype, "fabs", x)
Copy link
Member

Choose a reason for hiding this comment

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

Please also add support dispatching integer values, implement this function in the c++ side.

Expr tvm::abs(Expr x) {
   if (x.type().is_int()) {
      return select(x >= make_zero(x.type()), x, -x);
   } else if (x.type().is_float()) {
     return call_pure_intrin("fabs")...
   } else if (x.type().is_uint()) {
      return x;
   }
}

Copy link
Member

Choose a reason for hiding this comment

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

@@ -35,6 +35,7 @@ TOPI_DECLARE_UNARY_OP(floor);
TOPI_DECLARE_UNARY_OP(ceil);
TOPI_DECLARE_UNARY_OP(round);
TOPI_DECLARE_UNARY_OP(trunc);
TOPI_DECLARE_UNARY_OP(fabs);
Copy link
Member

Choose a reason for hiding this comment

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

topi function name needs to be consistent with numpy, absolute.

@tqchen tqchen added status: need update need update based on feedbacks status: review in progress labels Jul 9, 2018
@PariksheetPinjari909
Copy link
Contributor Author

@masahi @zhreshold Can you please review.

@PariksheetPinjari909
Copy link
Contributor Author

@tqchen thanks for review comments, i will work on it and update.

@zhreshold
Copy link
Member

Is there an abs alias for absolute? Or did you mean only floating point is supported since you are using fabs

@PariksheetPinjari909
Copy link
Contributor Author

@zhreshold numpy has aliased 'np.abs' and 'np.absolute'. Now I have kept same name 'abs' to be consistent with numpy, TF, mxnet, onnx.

@PariksheetPinjari909
Copy link
Contributor Author

@tqchen I have addressed review comments, pls review again.

@@ -71,6 +70,24 @@ inline Expr pow(Expr x, Expr y) {
return ir::Call::make(x.type(), "pow", { x, y }, ir::Call::PureIntrinsic);
}

/*!
* \brief Calculate absolute value of x, elementwise
* \param x The input data
Copy link
Member

Choose a reason for hiding this comment

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

add \return

y : Expr
The result.
"""
return call_pure_intrin(x.dtype, "fabs", x)
Copy link
Member

Choose a reason for hiding this comment

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

need to call abs c++ function here, instead of calling intrinsic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you guide me, how can i make a call from tvm python to tvm cpp function? Can you give me any example?

Copy link
Contributor

Choose a reason for hiding this comment

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

tvm.abs !!

Copy link
Member

Choose a reason for hiding this comment

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

you will need to register functions, and they will appear on the registered namespace. Checkout e.g. https://github.com/dmlc/tvm/blob/master/src/api/api_ir.cc#L76 try register abs under make.abs, and call it from there

@PariksheetPinjari909
Copy link
Contributor Author

@tqchen I have registered absolute op as make.abs in api_ir.cc. Pls check.

@tqchen tqchen merged commit 3887886 into apache:master Jul 13, 2018
@tqchen
Copy link
Member

tqchen commented Jul 13, 2018

Thanks @srkreddy1238 @zhreshold for code reviews and @PariksheetPinjari909 for contributing the code, this is now merged!

tqchen pushed a commit to tqchen/tvm that referenced this pull request Aug 4, 2018
sergei-mironov pushed a commit to sergei-mironov/tvm that referenced this pull request Aug 8, 2018
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.

None yet

4 participants