-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[QNN] Register a bunch of unary elementwise ops #10086
[QNN] Register a bunch of unary elementwise ops #10086
Conversation
This is now ready for review, cc @anwang2009 @masahi @mbrookhart @anijain2305 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, I've been thinking that we should make "all" ops qnn-aware by auto-generating default lowering like this, so that we don't have to decide which op can run on int8.
@@ -520,8 +525,8 @@ inline Expr FastSoftmax(Expr e, tvm::Attrs attr) { | |||
return Call(op, {e}, attr); | |||
} | |||
|
|||
inline Expr Log(Expr e) { | |||
static const Op& op = Op::Get("log"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, didn't know it was originally there.
At first, I added like 20 unary funcs but decided to scale it back to make review easier (and most of the unary funcs probably have little use). Didn't realize Log was already in pattern_utils
so probably deleted it on accident
src/relay/qnn/op/op_common.h
Outdated
* | ||
* FloatingPointFunc is usually a handle from "src/relay/transforms/pattern_utils.h" | ||
* | ||
* \param OpName the name of registry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: update this to "FloatingPointFunc" description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
relay.qnn.op.sqrt, | ||
np.sqrt, | ||
input_dtype="int8", | ||
x_data=np.arange(1, 128, dtype="int8"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason this test has x_data
specified but the other int8 tests don't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's sqrt so we want to keep things in domain. Outside of the domain the function can return really anything (probably either 0 or max/min value of int8)
src/relay/qnn/op/op_common.h
Outdated
for (size_t i = 1; i < 5; ++i) { \ | ||
types.push_back(arg_types[i]); \ | ||
} \ | ||
auto dequantized_arg = Dequantize(args.x, args.scale, args.zero_point, types, -1); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Dequantize -> DequantizeLower expects types
to start with the input data type, but in this code types
starts with the scale type.
https://github.com/apache/tvm/blob/main/src/relay/qnn/op/dequantize.cc#L99-L105
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, you appear to be right. Moving to using the MakeDequantize and MakeQuantize to avoid issues with getting type right
@anwang2009 PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. thanks!
a473635
to
5456a51
Compare
cc @masahi we cool with merging this? |
* 0;276;0cinitial commit * register a bunch of ops * unary ops * add a bunch of tests * 0;276;0crefactor tests * add tests to qnn * comments on macros * add back in log to pattern utils * update floating point func description * proper creating of calls to quantize and dequantize * fix lowering process for using dequantize and quantize ops
Adds a way to quickly add unary operators to QNN with default canonicalizations.
Also adds following: