Skip to content

Add onnx GridSample support for border padding mode #3819

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

Closed
wants to merge 7 commits into from

Conversation

Ax9D
Copy link

@Ax9D Ax9D commented Oct 25, 2024

  • Adds support for padding_mode="border"

  • Clamps the grid coordinates between 0 and size - 1 like this when using this padding mode:

x_result = min(max(0, x), W - 1)
y_result = min(max(0, y), H - 1)
  • Added Lit tests for both TorchToLinalg and OnnxToTorch lowerings

Copy link
Collaborator

@zjgarvey zjgarvey left a comment

Choose a reason for hiding this comment

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

Thanks for adding this support!

I have a couple of changes to request, and I'd also like you to sync your branch with head of main to avoid the failing CI.

Comment on lines 2577 to 2578
Value zeroInt =
b.create<arith::ConstantOp>(loc, b.getIntegerAttr(int64type, 0));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to keep this constant outside the lambda function, and just get it from the capture (otherwise you will re-write the constant multiple times in the IR).

Copy link
Author

Choose a reason for hiding this comment

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

Since paddingMode is checked at compile time, I have removed zeroInt completely.

@Ax9D
Copy link
Author

Ax9D commented Nov 14, 2024

Thanks for the feedback! I'll work on the requested changes.

@@ -2587,10 +2587,30 @@ class ConvertAtenGridSamplerOp : public OpConversionPattern<AtenGridSamplerOp> {
return res;
};

auto lambdaBorder = [&](OpBuilder &b, Location loc, Value x,
Value SizeSubOne) -> Value {
Copy link
Collaborator

Choose a reason for hiding this comment

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

SizeSubOne -> sizeSubOne

auto resultType = cast<RankedTensorType>(
getTypeConverter()->convertType(op.getResult().getType()));
Value alignCorners = adaptor.getAlignCorners();
Value interMode = adaptor.getInterpolationMode();

int64_t paddingModeInt;
matchPattern(op.getPaddingMode(), m_TorchConstantInt(&paddingModeInt));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not put this inside an if and return failure?

@vivekkhandelwal1
Copy link
Collaborator

@Ax9D Why did you close this PR and open a new one here: #4186?

The changes are not so large that you face an issue in rebasing this PR. Please reopen this and close the other one. It becomes hard to follow the review comments.

@Ax9D
Copy link
Author

Ax9D commented May 19, 2025

I completely agree with you, but I accidentally removed the branch and the PR was automatically closed, and I can't seem to open it again, even though the branch is restored.... Is it due to the new one being open?

@vivekkhandelwal1
Copy link
Collaborator

vivekkhandelwal1 commented May 27, 2025

I completely agree with you, but I accidentally removed the branch and the PR was automatically closed, and I can't seem to open it again, even though the branch is restored.... Is it due to the new one being open?

I think that's happening probably because the link between your past and the restored one is lost. Anyway, you should open any of the PR which you are able to.

@Ax9D
Copy link
Author

Ax9D commented Jun 13, 2025

Hey, I reopened the other one. Sorry for the inconvenience.

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