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

Add support for integer division to TCP #97

Merged
merged 6 commits into from
Sep 20, 2024

Conversation

srinathava
Copy link
Contributor

@srinathava srinathava commented Sep 11, 2024

As titled, add support for integer division in TCP. This closely mimics the existing capabilities in pytorch and arith.

@srinathava srinathava marked this pull request as ready for review September 12, 2024 14:31
Comment on lines 173 to 174
Tcp_SignednessAttr:$signedness,
Tcp_RoundingModeAttr:$rounding_mode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need a "Ceil" rounding mode here? Are they any cases where that is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's a good use-case for the tcp.diviop itself to support a ceil rounding mode, given that such a thing does not exist for the torch.div op. I added it because there is a natural mapping to the arith.ceildiviop. I can remove it.

However, I think the RoundingMode type itself should have ceil in it for completion though.

Copy link
Collaborator

@navahgar navahgar left a comment

Choose a reason for hiding this comment

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

LGTM. Some comments inline.

@@ -32,4 +32,22 @@ def Tcp_Signedness : I32EnumAttr<"Signedness",

def Tcp_SignednessAttr : EnumAttr<Tcp_Dialect, Tcp_Signedness, "signedness">;

// TCP rounding mode
def Tcp_RoundingMode_Trunc : I32EnumAttrCase<"Trunc", 0>;
def Tcp_RoundingMode_Floor : I32EnumAttrCase<"Floor", 1>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor nit: fix spacing here and below

return {b.create<arith::DivSIOp>(loc, payloadArgs[0], payloadArgs[1])};
else if (divOp.getRoundingMode() == RoundingMode::Ceil)
return {
b.create<arith::CeilDivUIOp>(loc, payloadArgs[0], payloadArgs[1])};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be arith::CeilDivSIOp instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yikes :( thanks for the typo fix.

let arguments = (ins
Tcp_IntTensor:$in1,
Tcp_IntTensor:$in2,
Tcp_RoundingModeAttr:$rounding_mode
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 the rounding mode here? Should we just stick to using trunc mode for divui?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that the arith dialect has straightforward mapping to ceildiv, why not just have it for the sake of expressibility? I do not have a strong opinion on this though. We could just leave it out now and introduce it later if we find a need for it.

@srinathava srinathava merged commit 3fc3290 into cruise-automation:main Sep 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants