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

[Opt] Improve the binary ops simplify pass #1646

Merged
merged 3 commits into from
Aug 6, 2020

Conversation

xumingkuan
Copy link
Contributor

@xumingkuan xumingkuan commented Aug 5, 2020

Related issue = #1226 #656

  • Do this optimization when fast_math=False and the operands are (return value is) of integral types.
  • Add (a - b) + c -> a - (b - c), (a - b) - c -> a - (b + c), (a / b) * c -> a / (b / c), (a / b) / c -> a / (b * c).
  • Disable (a / b) * c -> a / (b / c), (a * b) / c -> a * (b / c) when the data type is integral.
  • Let this pass return true when two operands are swapped but modifier.modify_ir() returns false.

Benchmark (#stmts): unchanged.

[Click here for the format server]


@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #1646 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1646   +/-   ##
=======================================
  Coverage   44.40%   44.40%           
=======================================
  Files          41       41           
  Lines        5876     5876           
  Branches     1022     1022           
=======================================
  Hits         2609     2609           
  Misses       3118     3118           
  Partials      149      149           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29071b3...39488c5. Read the comment docs.

@TH3CHARLie
Copy link
Collaborator

It's a nice improvement to the IR and optimization system.

  • Add (a - b) + c -> a - (b - c), (a - b) - c -> a - (b + c), (a / b) * c -> a / (b / c), (a / b) / c -> a / (b * c).

For the (a / b) * c -> a / (b / c) case, it can not be safely swapped due to the integer division roundoff. Consider a = 10, b = 3 and c = 3, the former produces 9 while the swapped one produces 10.

@xumingkuan
Copy link
Contributor Author

xumingkuan commented Aug 6, 2020

It's a nice improvement to the IR and optimization system.

  • Add (a - b) + c -> a - (b - c), (a - b) - c -> a - (b + c), (a / b) * c -> a / (b / c), (a / b) / c -> a / (b * c).

For the (a / b) * c -> a / (b / c) case, it can not be safely swapped due to the integer division roundoff. Consider a = 10, b = 3 and c = 3, the former produces 9 while the swapped one produces 10.

Oh yes. I disabled this optimization when the return type is integral. And IIRC our typecheck guarantees that binary operations have the same type among operands and the return value.

Copy link
Collaborator

@TH3CHARLie TH3CHARLie left a comment

Choose a reason for hiding this comment

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

LGTM, just update the PR description about disabling integral division.

@xumingkuan xumingkuan merged commit 08f1ef8 into taichi-dev:master Aug 6, 2020
@xumingkuan xumingkuan deleted the binary branch August 7, 2020 08:30
@yuanming-hu yuanming-hu mentioned this pull request Aug 8, 2020
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