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

Specification of quantized reduce and other related ops #1796

Merged
merged 9 commits into from
Dec 6, 2023

Conversation

sdasgup3
Copy link
Member

@sdasgup3 sdasgup3 commented Oct 9, 2023

The PR implements the approved RFC for reduce op by proposing the specification related changes for reduce, reduce_window and select_and_scatter ops.

In #1647, we talked about some of the other ops which will depend on the quantized specification of reduce op. Initially, I thought about creating separate PRs for them, but for the interest of time and the fact that their handling is going to be very similar to how reduce op is handled, I propose to include their PR in the current PR.

Here are the additional ops (other than reduce, reduce_window, select_and_scatter) whose specification is added

  • Ops with explicit computation regions: all_reduce, reduce_scatter, scatter: They are handled similar to how reduce op is handled.

  • Ops w/o explicit computation region: batch_norm_grad, batch_norm_training: For these ops, the semantics of the operation implicitly does reduction with a custom computation function. As there is no explicit computation function in the IR, the proposal in the RFC cannot be applied. I propose (A) to handle these ops similar to how batch_norm_inference is handled with dequant-op-quant strategy, (B) we can revisit this op later if there is use case to do that implicit reduction using higher accumulation type.

One implementation detail: The fact that batch_norm_grad, batch_norm_training ops returns multiple outputs and current dequantize_op_quantize returns a single output, is handled using a dedicated meta function for these two ops.

Next steps: Once the PR is approved the plan is to propose the corresponding changes in the verifier/shape functions for these ops.

Only support promotion of reduction operands to reduction-block arguments
The specific question bought up in the discussion is: should we promote demotion as well? It seems that there aren't any use-cases for that. For example, consider the following cases of implicit conversion for reduction operations. The examples are shown using integer type but can be generalized to other types as well.

input element type accumulator element type result element type use-cases
i8 i8 i8 Reduction using min/max
i8 i32 i32 Reduction using average/sum
i8 i32 i8 Demotion from accumulation type to result type. (No use-case)
i8 i32 i64 Extra promotion from accumulation type to result type. (No use-case)
i32 i8 i8 Demotion from input value type to accumulation type. (No use case)

With that in mind, we propose to develop the specification based on the scenario we have the use-case for (just for promotion: none of the cases of demotion or extra promotion will be supported in the specification.

docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
@sdasgup3 sdasgup3 force-pushed the reduce-impl branch 3 times, most recently from 2d287df to 3debca6 Compare October 10, 2023 19:10
docs/spec.md Outdated Show resolved Hide resolved
@sdasgup3 sdasgup3 force-pushed the reduce-impl branch 2 times, most recently from a326fb5 to bfd96a2 Compare October 10, 2023 19:29
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
@sdasgup3 sdasgup3 changed the title Specification of reduce, reduce_window and select_and_scatter ops Specification of quantized reduce and other related ops Oct 12, 2023
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
@sdasgup3 sdasgup3 force-pushed the reduce-impl branch 4 times, most recently from 2d4f432 to ada3d64 Compare October 13, 2023 20:35
docs/spec.md Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
docs/spec.md Outdated Show resolved Hide resolved
@GleasonK
Copy link
Member

The semantics LGTM. Have a few suggestions to make the spec more readable.

Copy link
Member

@GleasonK GleasonK left a comment

Choose a reason for hiding this comment

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

LGTM. Let's resolve the last open comment and then I'll approve. Will reach out offline to discuss further.

@sdasgup3 sdasgup3 force-pushed the reduce-impl branch 2 times, most recently from 015f1d3 to 67fa1f1 Compare November 27, 2023 23:50
docs/spec.md Show resolved Hide resolved
docs/spec.md Show resolved Hide resolved
@sdasgup3 sdasgup3 merged commit c015a21 into openxla:main Dec 6, 2023
7 checks passed
sdasgup3 added a commit that referenced this pull request Dec 20, 2023
Implements the specification changes at
#1796.

The PR adds/updates  the verifier and type inference routines for the
following ops: `reduce, reduce_window, select_and_scatter, all_reduce,
reduce_scatter, scatter`. Please refer to #1796 for the updated
constraints which the PR implements. Note the #1796 is going to be
merged soon.

Here are the changes for each operation:

 - reduce 
     - #1796 added a new constraint C8 
     - Updated labels
- Add positive tests and negative tests verifying reduce_c6  at
verify_reduce.mlir  and type inference tests at infrer_stablehlo.mlir,
for reduce_c8.

  - reduce_window: 
     - #1796 updated the C16
- Add positive tests ; negative tests verifying reduce_window_c13  at
verify_reduce_window.mlir  and type inference tests at
infrer_stablehlo.mlir, for reduce_window_c16.

  - select_and_scatter
     - #1796 added a new constraint C12
     - Updated labels
- Add positive tests ; negative tests verifying selelct_and_scatter_c10 
at verify_select_and_scatter.mlir  and type inference tests at
infrer_stablehlo.mlir, for select_and_scatter_c12.

  - scatter
   - #1796 added a new constraint C17
   -   Updated labels
   - Add positive tests ; negative tests verifying scatter_c15  at
verify_scatter.mlir  and type inference tests at infrer_stablehlo.mlir,
for scatter_C17.

  - reduce_scatter
     - #1796 added a new constraint C9 
- This op does not have the type inference supported. We had a trait
`SameOperandsAndResultElementType` implementing the outdated constraint.
For the new constraint C9, we added a check at `verifyReduceScatterOp`. 
- Add positive tests; negative tests verifying reduce_scatter_C7  at
ops_stablehlo.mlir  and type inference tests at ops_stablehlo.mlir,
for reduce_scatter_C9.

  - all_reduce
     - #1796 added a new constraint C7
- This op implemented an outdated constraint related to type inference
using `inferReturnTypeComponentsFromOperands`. For the new constraint
C9, we added a trait `InferTensorType` in the tablegen definition of the
op. 
     - Updated labels 
- Add positive tests; negative tests verifying all_reduce_C5  at
ops_stablehlo.mlir  and type inference tests at ops_stablehlo.mlir,
for all_reduce_C7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants