Skip to content

Add an equality node - #4079

Merged
miladm merged 4 commits into
masterfrom
krovatkin/eq
Oct 14, 2022
Merged

Add an equality node#4079
miladm merged 4 commits into
masterfrom
krovatkin/eq

Conversation

@Krovatkin

Copy link
Copy Markdown
Contributor

No description provided.

const torch::lazy::DimensionNode* dim_node_1 = DimCast(operand(1));
XLA_CHECK(dim_node_0);
XLA_CHECK(dim_node_1);
return dim_node_0->getDynamicValue() == dim_node_1->getDynamicValue() ? 1 : 0;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

would return dim_node_0->getDynamicValue() == dim_node_1->getDynamicValue() caused a weird cast issue?

Comment thread torch_xla/csrc/ops/dynamic_ir.h
@JackCaoG

Copy link
Copy Markdown
Collaborator

Can you rebase and kick off CI again?

SizeEq(torch::lazy::Value a, torch::lazy::Value b);
int64_t getDynamicValue() const override;
int64_t getStaticValue() {
TORCH_CHECK(false, "Comparison operators should be using getDynamicValue");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess the use case for the SizeEq node is that users does a tensor1.shape[0] == tensor2.shape[0] or tensor1.shape[0] == tensor1.shape[1] or something similar, right?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

any two tensor shapes can be compared.

Comment thread torch_xla/csrc/ops/dynamic_ir.h
Comment thread torch_xla/csrc/tensor.cpp
return this->data()->ir_value->op() != xla_device_data;
}

c10::SymIntNode XLASymIntNodeImpl::eq(const c10::SymIntNode& other) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do you need to add a test?

@miladm miladm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Awesome. Took an initial pass. Reran the build.

@miladm miladm added the dynamism Dynamic Shape Features label Oct 11, 2022
Comment thread torch_xla/csrc/tensor.cpp Outdated
c10::SymIntNode XLASymIntNodeImpl::eq(const c10::SymIntNode& other) {
auto pother = dynamic_cast<XLASymIntNodeImpl*>(other.get());
auto neq = torch::lazy::MakeNode<SizeEq>(node(), pother->node());
return c10::make_intrusiveXLASymIntNodeImpl>(neq);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

make_intrusive<XLASymIntNodeImpl>

@miladm miladm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @Krovatkin

@miladm
miladm merged commit c5ff280 into master Oct 14, 2022
@miladm miladm linked an issue Oct 14, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dynamism Dynamic Shape Features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simple NN model test with dynamic shape breaks torch.nn.functional

4 participants