Consider removing tuples from CustomCallOp #598
Assignees
Labels
Comments
|
Ping on this? Isn't it the only thing that keeps tuple around? It'd simplify things from the spec (and the implementation) if we could finally finish eliminating these. |
|
Hi Mehdi (@joker-eph) Unfortunately, we did get the time to look into this because of the some other deliverables which we are planning for in the current quarter. ( which, for front-end contract tickets, mostly looks like the P0 and P1s in https://github.com/orgs/openxla/projects/6 ). With all that, I was planing to target this in sometime in next quarter. In any case, please let me know your opinion about the plan. |
|
It's great to see that this didn't go forgotten, thanks for the update! |
burmako
pushed a commit
that referenced
this issue
Jun 2, 2023
…ns (#1554) Data Movement operations were one of the first operations specced, so we were still working out the notation back then. As mentioned in #484, inspired by our experience with the interpreter, it appears to be useful to express semantics of these operations in terms of result_index / operand_index rather than in terms of individual components of those indices. I took a stab at tweaking the specification for these operations, and I think that going from scalars to tensors to represent indices results in more readable specification. Furthermore, descriptive names like result_index are more conducive to readability than i or j. Also, we can retire the somewhat weird "id" / "jd" notation for these ops. Overall, I think that the specification and the implementation of data movement ops are well-aligned with each other. A few quirks: * ConcatenateOp, PadOp and TransposeOp are formulated in terms of operand's iteration space because they are much easier to implement this way. Everything else is formulated in terms of result's iteration space. * SortOp doesn't have a low-level 1:1 mapping, but there's a 1:1 mapping of high-level pieces and comments help with understanding. Re: the rest of the opset (see the list of categories in https://docs.google.com/spreadsheets/d/1rvhxQMFUtCZ5DsY6X0_lJOCg9rVO2MdyeZlRorsc0UI/edit?resourcekey=0-5gMjnlkXDL6hCntv2yltaQ#gid=0): * Control Flow ops look good as well (modulo a minor simplification in #1553). * Data Movement ops are discussed in this PR. * Distribution ops are in the works and will be reviewed afterwards. * Dynamism ops are in the works as well, and we'll first need to spec them before worrying about the interpreter. Overall, I'm not worried because at runtime there's not going to be a difference between static ops and their dynamic counterparts. * Elementwise ops are straightforward as far as notation goes. * Extensibility ops are out of scope for now because we may decide to remove tuples altogether (#598). * Most of the Miscellaneous ops are going to be handled via passes in the long run, so I haven't reviewed those. The rest, i.e. constant and iota, are fine with a minor fix to iota. * Not in HLO ops are going to be out of the opset in the long run, so we haven't specced them and aren't planning to spec or implement them. * Quantization ops are elementwise, so they are straightforward too as far as notation goes. * Finally, Reduction ops are going to be tricky because their specs have been tricky, their implementations have been tricky, and their notations have been drifting apart. I opened #1551 to revisit this once we're done with the first round of implementations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
From the spec: "Tuple types are inherited from HLO where they are used to model variadic inputs and outputs. In StableHLO, variadic inputs and outputs are supported natively, so the only use of tuple types in StableHLO is in custom_call where tuple types are used to model HLO-compatible ABI of custom calls".
Given that, it would be highly desirable to remove tuples from CustomCallOp. The only concern is that this will reduce the expressive power of the op and make it incompatible with XLA. We have done some initial analysis and came to the conclusion that the only potential problem here would be XLA:CPU and that we only anticipate minor breakages. It will very likely be worth it to go through with this change.
The text was updated successfully, but these errors were encountered: