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

Support ScatterOp Codegen #1270

Closed
wants to merge 1 commit into from
Closed

Conversation

eedalong
Copy link
Collaborator

@eedalong eedalong commented Nov 8, 2023

No description provided.

@CLAassistant
Copy link

CLAassistant commented Nov 8, 2023

CLA assistant check
All committers have signed the CLA.

@eedalong eedalong force-pushed the support_scatter branch 4 times, most recently from 0664a4e to 91d3a3f Compare November 8, 2023 11:18
@eedalong eedalong changed the title [WIP] Add scatter op lowering unit test [WIP] Support ScatterOp Codegen Nov 9, 2023
@Yancey1989 Yancey1989 force-pushed the support_scatter branch 2 times, most recently from 3505b6d to 72b23e5 Compare November 9, 2023 09:13
@Yancey1989 Yancey1989 force-pushed the support_scatter branch 8 times, most recently from e0fad5d to 6e778a0 Compare November 24, 2023 06:19
@Yancey1989 Yancey1989 force-pushed the support_scatter branch 2 times, most recently from a83f009 to dadfe54 Compare November 29, 2023 09:43
@eedalong eedalong changed the title [WIP] Support ScatterOp Codegen Support ScatterOp Codegen Nov 29, 2023
@eedalong eedalong requested a review from Yancey1989 November 29, 2023 09:45
@Yancey1989 Yancey1989 force-pushed the support_scatter branch 4 times, most recently from b8b5e2a to 422fc9a Compare November 29, 2023 10:53
auto sliceOpResultType = RankedTensorType::get(indexType.getShape(), srcType.getElementType());
src = rewriter.create<mhlo::RealDynamicSliceOp>(loc, getTypeConverter()->convertType(sliceOpResultType), src, baseIndicesValue, limitIndicesValue, stridesValue);

// Construct ScatterDimensionNumbersAttr
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplicate comments with L1595 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed


// Reference implementation: https://github.com/pytorch/xla/blob/master/torch_xla/csrc/xla_lower_util.cpp#L139
template <>
LogicalResult ConvertAtenOp<AtenScatterSrcOp>::matchAndRewrite(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this rewriter function is too long, can you add some pesucode to describe how to lower the AtenScatterOp briefly?

@@ -295,5 +295,26 @@ def LHLO_ArgsMutationOp : LHLODISC_Op<"args_mutation", []> {
);
}

def LHLODISC_InplaceScatterOp: LHLODISC_Op<"inplace_scatter", []> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need a new operator to define a INPLACE op? Maybe we simple way is if (op.operand[0] == op.operand[-1]) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah you are right. Fixed.

output_placements = "cpu",
outputs = "output0"
}} {
%2 = "mhlo.scatter"(%arg0, %arg1, %arg2) ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

this ut is too simple, please check the IR of scatter op lowing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@Yancey1989 Yancey1989 force-pushed the support_scatter branch 3 times, most recently from c7c9589 to d1eece4 Compare December 7, 2023 07:15
@eedalong eedalong requested a review from Yancey1989 December 7, 2023 07:49
@Yancey1989 Yancey1989 force-pushed the support_scatter branch 2 times, most recently from bb5c6e6 to 799b5b7 Compare December 7, 2023 11:58
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.

3 participants