Skip to content

Conversation

@wonjoo-wj
Copy link
Collaborator

@wonjoo-wj wonjoo-wj commented May 17, 2022

Replace XlaValue with upstream lazy::Value

Following up on #3567, we see that keeping our own torch_xla::XlaValue causes issues for LTC codegen and will make things unnecessarily more complicated. This removes our torch_xla::XlaValue completely and uses the upstream lazy::Value.

This PR adds a function GetXlaShape(lazy::Value) in ir.h that retrieves the xla shape given a lazy::Value, which eliminates the need to keep our own XlaValue.

@wonjoo-wj
Copy link
Collaborator Author

Test failing with:

test_memory_format_operators_xla (__main__.TestTorchDeviceTypeXLA) ... 2022-05-17 08:44:27.570902: F     920 tensorflow/compiler/xla/shape_util.cc:802] TUPLE primitive type has no definitive size
*** Received signal 6 ***

*** Begin stack trace ***
	tensorflow::CurrentStackTrace[abi:cxx11]()
	
	
	gsignal
	abort
	
	xla::ShapeUtil::ByteSizeOfPrimitiveType(xla::PrimitiveType)
	torch_xla::XlaHelpers::PromoteType(xla::PrimitiveType, xla::PrimitiveType)
	torch_xla::XlaHelpers::GetPromotedBinaryOpShape(xla::Shape const&, xla::Shape const&)
	torch_xla::operator*(torch::lazy::Value const&, torch::lazy::Value const&)

Looking into it.

@wonjoo-wj wonjoo-wj self-assigned this May 17, 2022
@wonjoo-wj
Copy link
Collaborator Author

@JackCaoG, this should be ready for a quick review (although it's very long 😢 ), thanks!

Copy link
Collaborator

@JackCaoG JackCaoG left a comment

Choose a reason for hiding this comment

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

Thanks, I think we can merge this one first given the size of the pr and fix the review comment in the next pr.

// an array of torch::lazy::Value while we uses XlaValue.
// an array of torch::lazy::Value while we uses torch::lazy::Value.
for (auto& operand : operands) {
AddOperand(operand.node, operand.index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact I think this can be changed. Since now we also use torch::lazy::Value, we don't need to manually call AddOperand, we can reuse upstream constructor to do that. @wonjoolee95 Can you fix that in a follow up pr?

};

using OpList = absl::Span<const XlaValue>;
using OpList = absl::Span<const torch::lazy::Value>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can also use upstream OpList I think, no need to redefine it here.

@JackCaoG
Copy link
Collaborator

@miladm FYI, this will likely cause a lot of conflicts.

@JackCaoG JackCaoG merged commit ec8a744 into master May 18, 2022
@JackCaoG JackCaoG deleted the value branch May 18, 2022 18:40
@wonjoo-wj
Copy link
Collaborator Author

Thanks, I think we can merge this one first given the size of the pr and fix the review comment in the next pr.

Thanks for the comments! Created a follow-up PR here #3583. Waiting on the CI to verify the tests.

@wonjoo-wj wonjoo-wj added the tracing Lazy Tensor tracing label Sep 27, 2022
ysiraichi added a commit that referenced this pull request May 22, 2025
- `SgnOp` and `SignOp`
    - Full codegen migration: #3577
    - Mistakenly re-introduced: #3572
- `LogSigmoid`
    - Introduced: #3539
    - Full codegen migration: #3743
- `SiLU`
    - Introduced: #2721
    - Full codegen migration: #3780
- `SiLUBackward`
    - Introduced: #3195
    - Full codegen migration: #3780
- `SeLU`
    - Introduced: #3547
    - Full codegen migration: #3780
- `Sigmoid`
    - Introduced: 6a73deb (no PR record)
    - Full codegen migration: #6342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tracing Lazy Tensor tracing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants