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

support lowering for aten.log1p #93

Merged
merged 3 commits into from
Sep 3, 2024

Conversation

zezhang
Copy link
Contributor

@zezhang zezhang commented Aug 31, 2024

Lower aten.log1p op to tcp.log(tcp.add(input, 1.0))

To test:

bazel test //... (in docker)

Copy link
Collaborator

@jain-sambhaav jain-sambhaav left a comment

Choose a reason for hiding this comment

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

Can you add this to the aot_compile test suite for e2e coverage?

@zezhang zezhang changed the title support lowering or aten.log1p support lowering for aten.log1p Sep 3, 2024
Copy link
Collaborator

@sjain-stanford sjain-stanford left a comment

Choose a reason for hiding this comment

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

LGTM to land after this is added to aot_compile suite.

@zezhang
Copy link
Contributor Author

zezhang commented Sep 3, 2024

Can you add this to the aot_compile test suite for e2e coverage?

Just added.

@sjain-stanford
Copy link
Collaborator

sjain-stanford commented Sep 3, 2024

There are two parity failures.

[ RUN      ] AotCompiled.ExecuteTest

bazel-out/k8-fastbuild/bin/test/AotCompile/_internal_log1p_execute_test.cpp:139: Failure
Expected equality of these values:
  Result.Output0.data[i]
    Which is: -0.0529[184](https://github.com/cruise-automation/mlir-tcp/actions/runs/10687447249/job/29625079316#step:9:185)45
  refOutput0.data<float>()[i]
    Which is: -0.052918427

bazel-out/k8-fastbuild/bin/test/AotCompile/_internal_log1p_execute_test.cpp:139: Failure
Expected equality of these values:
  Result.Output0.data[i]
    Which is: -nan
  refOutput0.data<float>()[i]
    Which is: -nan

We use EXPECT_FLOAT_EQ which checks two fp numbers are equal within 4 ULPs. Since this exceeds it, we might want to use a custom tolerance maybe with EXPECT_NEAR? Also not sure how this works for nans.

@zezhang
Copy link
Contributor Author

zezhang commented Sep 3, 2024

There are two parity failures.

[ RUN      ] AotCompiled.ExecuteTest

bazel-out/k8-fastbuild/bin/test/AotCompile/_internal_log1p_execute_test.cpp:139: Failure
Expected equality of these values:
  Result.Output0.data[i]
    Which is: -0.0529[184](https://github.com/cruise-automation/mlir-tcp/actions/runs/10687447249/job/29625079316#step:9:185)45
  refOutput0.data<float>()[i]
    Which is: -0.052918427

bazel-out/k8-fastbuild/bin/test/AotCompile/_internal_log1p_execute_test.cpp:139: Failure
Expected equality of these values:
  Result.Output0.data[i]
    Which is: -nan
  refOutput0.data<float>()[i]
    Which is: -nan

We use EXPECT_FLOAT_EQ which checks two fp numbers are equal within 4 ULPs. Since this exceeds it, we might want to use a custom tolerance maybe with EXPECT_NEAR? Also not sure how this works for nans.

Right.... from PyTorch Website Looks like log1p is better used with small input values.

@sjain-stanford
Copy link
Collaborator

sjain-stanford commented Sep 3, 2024

Yeah you might want to try constraining the inputs to be positive and small (instead of torch.randn in the loader) since log(1+x) is only defined for 1+x > 0.

@zezhang zezhang merged commit 29062f8 into cruise-automation:main Sep 3, 2024
1 check passed
@zezhang zezhang deleted the zezhang/aten_log1p branch September 3, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants