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.

Sign up for GitHub

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

Jump to bottom

[TOSA] : Fix float to integer cast for torch.ops.aten.to lowering. #3946

Merged
merged 6 commits into from
Jan 22, 2025
Merged

[TOSA] : Fix float to integer cast for torch.ops.aten.to lowering. #3946

merged 6 commits into from
Jan 22, 2025

Conversation

sahas3
Copy link
Member

@sahas3 sahas3 commented Jan 8, 2025

The behavior of float -> integer cast in PyTorch (though I haven't found the actual code implementing the cast) appears to be (based on the results produced in PyTorch):

  1. round the float nearest to zero (similar to arith.fptosi/ui)
  2. then perform the conversion

Currently we only emit tosa.cast for this operation but as per the spec https://www.mlplatform.org/tosa/tosa_spec.html#_cast the rounding performed for float -> integer is round to nearest integer (not zero). Hence, the current TOSA lowering for torch.ops.aten.to produces incorrect answer.

@sahas3 sahas3 requested a review from sjarus January 8, 2025 15:10
@justin-ngo-arm
Copy link
Contributor

Hi @sahas3, for the Torch float to integer cast behavior, did you observe it to be the same for all Torch casting, or just torch.aten.to? This is because I plan to make tosaCastTensorToType the universal tosa.cast call within TorchToTosa (plan described in #3948), and if the behavior you observed is specific to aten.to then I might need to re-evaluate my approach to unifying these calls.

@sahas3
Copy link
Member Author

sahas3 commented Jan 10, 2025

Hi @sahas3, for the Torch float to integer cast behavior, did you observe it to be the same for all Torch casting, or just torch.aten.to? This is because I plan to make tosaCastTensorToType the universal tosa.cast call within TorchToTosa (plan described in #3948), and if the behavior you observed is specific to aten.to then I might need to re-evaluate my approach to unifying these calls.

I think torch.aten.to or torch.tensor.to ops are the basis of all casts in PyTorch. I haven't seen different behavior in my experiments yet. Some additional tests also started passing with this PR indicating that the cast behavior is same for other cast like ops too. So I think using tosaCastTensorToType universally for all tosa.cast calls is the correct approach.

sjarus
sjarus reviewed Jan 14, 2025
View reviewed changes
Copy link
Collaborator

@sjarus sjarus left a comment

Choose a reason for hiding this comment

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

If Torch mandates round towards zero, can that not be synthetically expressed here @sahas3 ?

@sahas3
Copy link
Member Author

sahas3 commented Jan 14, 2025

If Torch mandates round towards zero, can that not be synthetically expressed here @sahas3 ?

Hi @sjarus , can you elaborate more on what you mean by synthetically expressing rounding towards zero?

@sjarus
Copy link
Collaborator

sjarus commented Jan 22, 2025

Looks like we had a misunderstanding earlier @sahas3 . Approving.

@sahas3 sahas3 merged commit 481da8d into llvm:main Jan 22, 2025
3 checks passed
justin-ngo-arm added a commit to justin-ngo-arm/torch-mlir that referenced this pull request Feb 4, 2025
@justin-ngo-arm
[TOSA] Refactor: Use tosaCastTensorToType() function to create tosa.cast
da60c17 
* Previously, in Torch to TOSA, there are 3 ways to create tosa.cast op:
  - `rewriter.create<tosa::CastOp>()`
  - `tosa::promoteType()`
  - `tosa::tosaCastTensorToType()`
* This commit combines the three APIs above into
  `tosa::tosaCastTensorToType()` with the following features:
  - Checking whether source and destination element types are the same
    before casting. If they are same, skip the cast.
  - Custom float to integer cast behavior added from this PR:
    llvm#3946
    TLDR: PyTorch's and TOSA's float to integer casting behaviors are
    different (round to zero vs round to nearest, respectively), which
    requires a custom casting here.
  - Future `TODO`: add a --strict mode which includes
    `checkValidityOfCast()` to ensure that the casting pairs follow TOSA
    specifications.
* Update LIT tests.

Signed-off-by: Justin Ngo <[email protected]>
Change-Id: I2aef3c79d8f2d98b93e671d5b815b8eab33e697e
justin-ngo-arm added a commit to justin-ngo-arm/torch-mlir that referenced this pull request Feb 11, 2025
@justin-ngo-arm
[TOSA] Refactor: Use tosaCastTensorToType() function to create tosa.cast
1572148 
* Previously, in Torch to TOSA, there are 3 ways to create tosa.cast op:
  - `rewriter.create<tosa::CastOp>()`
  - `tosa::promoteType()`
  - `tosa::tosaCastTensorToType()`
* This commit combines the three APIs above into
  `tosa::tosaCastTensorToType()` with the following features:
  - Checking whether source and destination element types are the same
    before casting. If they are same, skip the cast.
  - Custom float to integer cast behavior added from this PR:
    llvm#3946
    TLDR: PyTorch's and TOSA's float to integer casting behaviors are
    different (round to zero vs round to nearest, respectively), which
    requires a custom casting here.
  - Future `TODO`: add a --strict mode which includes
    `checkValidityOfCast()` to ensure that the casting pairs follow TOSA
    specifications.
* Update LIT tests.

Signed-off-by: Justin Ngo <[email protected]>
Change-Id: I2aef3c79d8f2d98b93e671d5b815b8eab33e697e
vivekkhandelwal1 pushed a commit to vivekkhandelwal1/torch-mlir that referenced this pull request Feb 11, 2025
@justin-ngo-arm @vivekkhandelwal1
[TOSA] Refactor: Use tosaCastTensorToType() function to create tosa.cast
29976a9 
* Previously, in Torch to TOSA, there are 3 ways to create tosa.cast op:
  - `rewriter.create<tosa::CastOp>()`
  - `tosa::promoteType()`
  - `tosa::tosaCastTensorToType()`
* This commit combines the three APIs above into
  `tosa::tosaCastTensorToType()` with the following features:
  - Checking whether source and destination element types are the same
    before casting. If they are same, skip the cast.
  - Custom float to integer cast behavior added from this PR:
    llvm#3946
    TLDR: PyTorch's and TOSA's float to integer casting behaviors are
    different (round to zero vs round to nearest, respectively), which
    requires a custom casting here.
  - Future `TODO`: add a --strict mode which includes
    `checkValidityOfCast()` to ensure that the casting pairs follow TOSA
    specifications.
* Update LIT tests.

Signed-off-by: Justin Ngo <[email protected]>
Change-Id: I2aef3c79d8f2d98b93e671d5b815b8eab33e697e
vivekkhandelwal1 pushed a commit to vivekkhandelwal1/torch-mlir that referenced this pull request Feb 17, 2025
@justin-ngo-arm @vivekkhandelwal1
[TOSA] Refactor: Use tosaCastTensorToType() function to create tosa.cast
a55906e 
* Previously, in Torch to TOSA, there are 3 ways to create tosa.cast op:
  - `rewriter.create<tosa::CastOp>()`
  - `tosa::promoteType()`
  - `tosa::tosaCastTensorToType()`
* This commit combines the three APIs above into
  `tosa::tosaCastTensorToType()` with the following features:
  - Checking whether source and destination element types are the same
    before casting. If they are same, skip the cast.
  - Custom float to integer cast behavior added from this PR:
    llvm#3946
    TLDR: PyTorch's and TOSA's float to integer casting behaviors are
    different (round to zero vs round to nearest, respectively), which
    requires a custom casting here.
  - Future `TODO`: add a --strict mode which includes
    `checkValidityOfCast()` to ensure that the casting pairs follow TOSA
    specifications.
* Update LIT tests.

Signed-off-by: Justin Ngo <[email protected]>
Change-Id: I2aef3c79d8f2d98b93e671d5b815b8eab33e697e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjarus sjarus sjarus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sahas3 @justin-ngo-arm @sjarus