-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[Relax] Add FInferMixedPrecision and FRelaxInferLayout for conv transpose ops #18629
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
base: main
Are you sure you want to change the base?
[Relax] Add FInferMixedPrecision and FRelaxInferLayout for conv transpose ops #18629
Conversation
Summary of ChangesHello @guan404ming, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
2adc83b to
5756731
Compare
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.
Code Review
This pull request adds FInferMixedPrecision and FRelaxInferLayout support for conv1d_transpose and conv2d_transpose operators, which is a great addition for consistency and enabling optimizations. The implementations look correct and are accompanied by new tests. I've provided a few suggestions to improve code clarity and reduce duplication in the C++ implementation, and to enhance readability in the new Python tests.
| Layout desired_data_layout = (*it).second[0]; | ||
| Layout desired_weight_layout = (*it).second[1]; | ||
| Layout desired_output_layout = (*it).second.size() == 3 ? (*it).second[2] : (*it).second[0]; |
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.
There is repeated code for accessing desired layouts. To improve readability and maintainability, you can store the desired layout strings in local variables and reuse them. This avoids repeating (*it).second access and the ternary logic for the output layout. You can then use these variables on lines 730-732.
| Layout desired_data_layout = (*it).second[0]; | ||
| Layout desired_weight_layout = (*it).second[1]; | ||
| Layout desired_output_layout = (*it).second.size() == 3 ? (*it).second[2] : (*it).second[0]; |
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.
There is repeated code for accessing desired layouts. To improve readability and maintainability, you can store the desired layout strings in local variables at the beginning of the if block and reuse them. This avoids repeating (*it).second access and the ternary logic for the output layout. You can then use these variables to update new_attrs in lines 933-935 and 957-959.
| bb = relax.BlockBuilder() | ||
| x0 = relax.Var("x", R.Tensor((2, 3, 28), "float16")) | ||
| w0 = relax.Var("w", R.Tensor((3, 4, 3), "float16")) | ||
| x1 = relax.Var("x", R.Tensor((2, 3, 28), "int8")) | ||
| w1 = relax.Var("w", R.Tensor((3, 4, 3), "int8")) | ||
|
|
||
| _check_inference( | ||
| bb, | ||
| relax.op.nn.conv1d_transpose(x0, w0, out_dtype="float32"), | ||
| relax.TensorStructInfo((2, 4, 30), "float32"), | ||
| ) | ||
| _check_inference( | ||
| bb, | ||
| relax.op.nn.conv1d_transpose(x1, w1, out_dtype="int32"), | ||
| relax.TensorStructInfo((2, 4, 30), "int32"), | ||
| ) |
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.
For better readability, consider using more descriptive variable names that indicate the data type, like x_f16, w_f16, x_i8, and w_i8.
| bb = relax.BlockBuilder() | |
| x0 = relax.Var("x", R.Tensor((2, 3, 28), "float16")) | |
| w0 = relax.Var("w", R.Tensor((3, 4, 3), "float16")) | |
| x1 = relax.Var("x", R.Tensor((2, 3, 28), "int8")) | |
| w1 = relax.Var("w", R.Tensor((3, 4, 3), "int8")) | |
| _check_inference( | |
| bb, | |
| relax.op.nn.conv1d_transpose(x0, w0, out_dtype="float32"), | |
| relax.TensorStructInfo((2, 4, 30), "float32"), | |
| ) | |
| _check_inference( | |
| bb, | |
| relax.op.nn.conv1d_transpose(x1, w1, out_dtype="int32"), | |
| relax.TensorStructInfo((2, 4, 30), "int32"), | |
| ) | |
| bb = relax.BlockBuilder() | |
| x_f16 = relax.Var("x", R.Tensor((2, 3, 28), "float16")) | |
| w_f16 = relax.Var("w", R.Tensor((3, 4, 3), "float16")) | |
| x_i8 = relax.Var("x", R.Tensor((2, 3, 28), "int8")) | |
| w_i8 = relax.Var("w", R.Tensor((3, 4, 3), "int8")) | |
| _check_inference( | |
| bb, | |
| relax.op.nn.conv1d_transpose(x_f16, w_f16, out_dtype="float32"), | |
| relax.TensorStructInfo((2, 4, 30), "float32"), | |
| ) | |
| _check_inference( | |
| bb, | |
| relax.op.nn.conv1d_transpose(x_i8, w_i8, out_dtype="int32"), | |
| relax.TensorStructInfo((2, 4, 30), "int32"), | |
| ) |
| bb = relax.BlockBuilder() | ||
| x0 = relax.Var("x", R.Tensor((2, 3, 28, 28), "float16")) | ||
| w0 = relax.Var("w", R.Tensor((3, 4, 3, 3), "float16")) | ||
| x1 = relax.Var("x", R.Tensor((2, 3, 28, 28), "int8")) | ||
| w1 = relax.Var("w", R.Tensor((3, 4, 3, 3), "int8")) | ||
|
|
||
| _check_inference( | ||
| bb, | ||
| relax.op.nn.conv2d_transpose(x0, w0, out_dtype="float32"), | ||
| relax.TensorStructInfo((2, 4, 30, 30), "float32"), | ||
| ) | ||
| _check_inference( | ||
| bb, | ||
| relax.op.nn.conv2d_transpose(x1, w1, out_dtype="int32"), | ||
| relax.TensorStructInfo((2, 4, 30, 30), "int32"), | ||
| ) |
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.
For better readability, consider using more descriptive variable names that indicate the data type, like x_f16, w_f16, x_i8, and w_i8.
| bb = relax.BlockBuilder() | |
| x0 = relax.Var("x", R.Tensor((2, 3, 28, 28), "float16")) | |
| w0 = relax.Var("w", R.Tensor((3, 4, 3, 3), "float16")) | |
| x1 = relax.Var("x", R.Tensor((2, 3, 28, 28), "int8")) | |
| w1 = relax.Var("w", R.Tensor((3, 4, 3, 3), "int8")) | |
| _check_inference( | |
| bb, | |
| relax.op.nn.conv2d_transpose(x0, w0, out_dtype="float32"), | |
| relax.TensorStructInfo((2, 4, 30, 30), "float32"), | |
| ) | |
| _check_inference( | |
| bb, | |
| relax.op.nn.conv2d_transpose(x1, w1, out_dtype="int32"), | |
| relax.TensorStructInfo((2, 4, 30, 30), "int32"), | |
| ) | |
| bb = relax.BlockBuilder() | |
| x_f16 = relax.Var("x", R.Tensor((2, 3, 28, 28), "float16")) | |
| w_f16 = relax.Var("w", R.Tensor((3, 4, 3, 3), "float16")) | |
| x_i8 = relax.Var("x", R.Tensor((2, 3, 28, 28), "int8")) | |
| w_i8 = relax.Var("w", R.Tensor((3, 4, 3, 3), "int8")) | |
| _check_inference( | |
| bb, | |
| relax.op.nn.conv2d_transpose(x_f16, w_f16, out_dtype="float32"), | |
| relax.TensorStructInfo((2, 4, 30, 30), "float32"), | |
| ) | |
| _check_inference( | |
| bb, | |
| relax.op.nn.conv2d_transpose(x_i8, w_i8, out_dtype="int32"), | |
| relax.TensorStructInfo((2, 4, 30, 30), "int32"), | |
| ) |
5040008 to
f0ce8fb
Compare
Why
The
conv1d_transposeandconv2d_transposeoperators were missing FInferMixedPrecision and FRelaxInferLayout attribute implementations, which are needed for:How