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 dtype functions for ops that take dtype from 1st operand #1895

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

li-plus
Copy link
Collaborator

@li-plus li-plus commented Feb 22, 2023

This PR finished the task 1 of #1807.

The three crashing cases (ElementwiseClampModule_basic, IouOfModule_basic, NormalizeModule_basic) are related to clamp op whose dtype function accepts optional non-tensor arguments.

def aten〇clamp〡dtype(self_rank_dtype: Tuple[int, int], min: Optional[Union[int, float]] = None, max: Optional[Union[int, float]] = None) -> int:
    ...

They are crashing on the assert below. @ramiro050 do you have any idea for this?

// If the operand is NoneType, then we just need to derefine it to the
// optional type in the function signature.
if (operandType.isa<Torch::NoneType>()) {
assert(desiredType.isa<Torch::OptionalType>() &&
"Don't expect library functions to have NoneType parameters");
return b.create<DerefineOp>(loc, desiredType, operand).getResult();
}

@ramiro050 ramiro050 self-requested a review February 23, 2023 16:47
@ramiro050
Copy link
Collaborator

This seems to be an issue with our importer. If you look at the MLIR snippet generated for clamp, the type Optional[Union[int, float]] is being converted to !torch.union<int, float, none> instead of !torch.optional<union<int, float>>.

@ramiro050
Copy link
Collaborator

Actually, the type Union[int, float, None] is being created by torch.jit.script. I think it should simply be a matter of widening the types allowed by the assert to include union types that have None as one of the contained types

@li-plus li-plus force-pushed the dtype-1st-operand branch from 9622182 to a0863bc Compare April 2, 2023 10:24
@li-plus li-plus changed the title Add dtype functions for ops that take dtype from 1st operand WIP: Add dtype functions for ops that take dtype from 1st operand Apr 2, 2023
@li-plus li-plus force-pushed the dtype-1st-operand branch 3 times, most recently from 426ec60 to dba553f Compare April 2, 2023 11:54
@li-plus
Copy link
Collaborator Author

li-plus commented Apr 2, 2023

Actually, the type Union[int, float, None] is being created by torch.jit.script. I think it should simply be a matter of widening the types allowed by the assert to include union types that have None as one of the contained types

Thanks! This is exactly the case. I fixed it by derefining these optional scalars to type !torch.union<int, float, none>.

Once #1990 is merged, I will switch to meta tensors and resolve the WIP status.

@li-plus li-plus force-pushed the dtype-1st-operand branch from dba553f to 4ccb129 Compare April 10, 2023 13:52
@li-plus li-plus changed the title WIP: Add dtype functions for ops that take dtype from 1st operand Add dtype functions for ops that take dtype from 1st operand Apr 10, 2023
@li-plus
Copy link
Collaborator Author

li-plus commented Apr 10, 2023

@ramiro050 This PR is ready to merge. Would you review it? Thanks.

Copy link
Collaborator

@ramiro050 ramiro050 left a comment

Choose a reason for hiding this comment

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

Awesome!

@li-plus li-plus merged commit 6543db7 into llvm:dtype-functions-staging Apr 11, 2023
@li-plus li-plus deleted the dtype-1st-operand branch April 11, 2023 19:59
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