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

Generalize canonicalization pattern for more aten.sub/div/mul/add op #1209

Merged
merged 4 commits into from
Aug 16, 2022

Conversation

Vremold
Copy link
Collaborator

@Vremold Vremold commented Aug 10, 2022

This PR generalizes the canonicalization pattern in PR #935 to fit more AtenOps (like AtenSubTensorOp, AtenMulScalarOp, and AtenDivTensorModeOp). The basic idea is eliminating these tensor ops when their tensor operands are zero-ranked tensors, which come from ValueTensorLiteralOp or PrimNumToTensorScalarOp, and replacing them with corresponding scalar ops.
This PR also helps to solve issue #1182 .

@Vremold Vremold changed the title Generalize canonicalization pattern in for more sub/div/mul/add op Generalize canonicalization pattern in PR#935 for more aten.sub/div/mul/add op Aug 10, 2022
@ZihengJiang ZihengJiang changed the title Generalize canonicalization pattern in PR#935 for more aten.sub/div/mul/add op Generalize canonicalization pattern for more aten.sub/div/mul/add op Aug 10, 2022
Copy link
Contributor

@silvasean silvasean left a comment

Choose a reason for hiding this comment

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

I see a lot of repeated code in this PR. Can we have a function like this which handles all ops? I think it would make it easier to share all the code and would avoid the templates

LogicalResult rewrite0DTensorOp(Operation *op, ...) {
  // dyn_cast `op` to determine the operation.
}

@Vremold Vremold force-pushed the generalize-cononicalization-pattern branch from ff2a957 to 4bffccd Compare August 12, 2022 11:17
@Vremold
Copy link
Collaborator Author

Vremold commented Aug 12, 2022

I see a lot of repeated code in this PR. Can we have a function like this which handles all ops? I think it would make it easier to share all the code and would avoid the templates

LogicalResult rewrite0DTensorOp(Operation *op, ...) {
  // dyn_cast `op` to determine the operation.
}

Done!

@Vremold Vremold requested a review from silvasean August 15, 2022 14:43
lib/Dialect/Torch/IR/TorchOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/Torch/IR/TorchOps.cpp Show resolved Hide resolved
lib/Dialect/Torch/IR/TorchOps.cpp Outdated Show resolved Hide resolved
@Vremold Vremold force-pushed the generalize-cononicalization-pattern branch from 4bffccd to 4e24e08 Compare August 16, 2022 03:16
@Vremold Vremold merged commit 3b3cb99 into llvm:main Aug 16, 2022
@Vremold
Copy link
Collaborator Author

Vremold commented Aug 16, 2022

Merged. Thanks! @silvasean

@Vremold Vremold deleted the generalize-cononicalization-pattern branch August 16, 2022 05:28
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.

2 participants