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

Added support for torch arange float module #2749

Merged

Conversation

Abhishek-TyRnT
Copy link
Contributor

@Abhishek-TyRnT Abhishek-TyRnT commented Jan 13, 2024

Added Support for float dtype in in torch.arange in TOSA Dialect

This resolves the following issue :-
#2762

The following test cases are passing after this change

  1. ArangeDtypeIntModule_basic
  2. ArangeFloatModule_basic
  3. ArangeNegativeStartFloatModule_basic
  4. ArangeStartFloatModule_basic
  5. ArangeStartNegativeStepFloatModule_basic
  6. ArangeStartOutDtypeModule_basic
  7. ArangeStartStepFloatModule_basic

Copy link
Collaborator

@eric-k256 eric-k256 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@Abhishek-TyRnT
Copy link
Contributor Author

CI doesn't seem to pick up jobs... Is there any reason?

Copy link
Collaborator

@newling newling left a comment

Choose a reason for hiding this comment

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

This isn't going to work for large 64-bit integers, because casting to double will be lossy (mantissa of double is 52 bits, so 2^53 + 1 can't be represented as a double).

@Abhishek-TyRnT
Copy link
Contributor Author

You are right. I will try to find a way to fix this.

@Abhishek-TyRnT
Copy link
Contributor Author

I guess there will be no loss if we use long double instead of double. What do you think?

Copy link
Collaborator

@newling newling left a comment

Choose a reason for hiding this comment

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

Looks fine, just the final comment for the int case.

lib/Conversion/TorchToTosa/TorchToTosa.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@newling newling left a comment

Choose a reason for hiding this comment

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

The logic looks good to me. I think the code can be polished a little bit though :-)

lib/Conversion/TorchToTosa/TorchToTosa.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToTosa/TorchToTosa.cpp Outdated Show resolved Hide resolved
@Abhishek-TyRnT
Copy link
Contributor Author

Hi @newling. The int64 test suddenly started failing cause mlir couldn't typecast float32 or float64 to int64. So I manually typecasted them Torch-to-Tosa pass.

@Abhishek-TyRnT
Copy link
Contributor Author

The CI failure doesn't seem it to be a issue with TorchToTosa but something in stablehlo unexpectedly passing.

@newling
Copy link
Collaborator

newling commented Feb 20, 2024

Ok. You might need to update the xfail set? #2927
Have you rebased onto main?

@Abhishek-TyRnT
Copy link
Contributor Author

Abhishek-TyRnT commented Feb 21, 2024

I just checked, that test has been added there. It should hopefully pass now

@Abhishek-TyRnT
Copy link
Contributor Author

Now that it has passed. Can you please merge it. @newling

@newling
Copy link
Collaborator

newling commented Feb 26, 2024

Hi @Abhishek-TyRnT I've made a PR against this which should fix the link checks. It also makes some stylistic improvements (IMO) which you can consider pulling in if you like: Abhishek-TyRnT#1

@newling newling merged commit d541779 into llvm:main Feb 27, 2024
3 checks passed
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.

4 participants