Simplify and extend ValueRanges #104557

lezcano · 2023-07-03T22:27:01Z

Stack from ghstack (oldest at bottom):

This PR:

It adds a few boolean variants of some methods that were missing
It simplifies the implementation of plenty of the operations
Adds ModularIndexing to the SymPy interpreter

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @ngimel @yf225 @chenyang78 @kadeng @muchulee8

This PR: - It adds a few boolean variants of some methods that were missing - It simplifies the implementation of plenty of the operations - Adds ModularIndexing to the SymPy interpreter [ghstack-poisoned]

pytorch-bot · 2023-07-03T22:27:03Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/104557

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 19eb958:

NEW FAILURE - The following job has failed:

cuda12.1-py3.10-gcc9-sm86-periodic-dynamo-benchmarks / test (dynamo_eager_torchbench, 1, 1, linux.g5.4xlarge.nvidia.gpu) (gh)

This comment was automatically generated by Dr. CI and updates every 15 minutes.

This PR: - It adds a few boolean variants of some methods that were missing - It simplifies the implementation of plenty of the operations - Adds ModularIndexing to the SymPy interpreter cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 [ghstack-poisoned]

eellison

Nice !! Have a few comments.

Is it worth committing the fuzzing script that Nuno wrote https://gist.github.com/nunoplopes/dc08bee4f3fb3609f916aa1e9389273a ? Or something like it, maybe as a slow test.

eellison · 2023-07-06T16:42:49Z

torch/utils/_sympy/value_ranges.py

    lower: Union[sympy.Expr, SympyBoolean]
    upper: Union[sympy.Expr, SympyBoolean]
+    is_bool: bool


its a little funny that we're tracking the dtype only if it's a bool

Yeah. It's in my radar to improve it eventually to track the type properly and make sure that the type of the bounds and the type of the object is sound. For now, bool it is, given that booleans are just very different to floats and integers in sympy.

torch/utils/_sympy/value_ranges.py

eellison · 2023-07-06T17:09:38Z

torch/utils/_sympy/value_ranges.py

+    def truncdiv(cls, a, b):
+        x = cls.truediv(a, b)
+        if x == ValueRanges.unknown():
+            return x


is this equivalent, or fixing a bug ?

Equivalent and noticeably simpler.

eellison · 2023-07-06T17:10:54Z

torch/utils/_sympy/value_ranges.py

            if a == 0:
-                return 0
+                return a


jc, why did you have to return a here instead of 0?

to return a zero that's both a sympy object and it's of the correct dtype.

torch/utils/_sympy/value_ranges.py

eellison · 2023-07-06T17:16:42Z

torch/utils/_sympy/value_ranges.py

+        while b > 0:
+            if b % 2 == 1:
+                acc = cls.mul(acc, a)
+            a = cls.mul(a, a)
+            b = b // 2


Can we not just call into sympy.pow at this point ? Feel like there's probably a way of implementing this simpler

Yeah, let me rewrite this. This one's a weird op, as -3.0 ** x is not well defined, but -3 ** x is well defined for x integer. I'll split it into a integer and not.

Ok, I heavily simplified this logic, now it looks much better.

lezcano · 2023-07-06T18:19:04Z

For the testing, I plan to push some proper testing soonish, where we test these ranges on the FX interpreter, for the nodes that we use in that interpreter, and then keep Ed's tests for the SymPy nodes. I also plan to trim the SymPy interpreter to just implement the functions we currently use for the guards and the indexing on inductor.

torch/utils/_sympy/value_ranges.py

This PR: - It adds a few boolean variants of some methods that were missing - It simplifies the implementation of plenty of the operations - Adds ModularIndexing to the SymPy interpreter cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 [ghstack-poisoned]

lezcano · 2023-07-06T21:23:16Z

@eellison addressed the review

lezcano · 2023-07-06T22:45:21Z

The next two are also ready and should be quite easy reviews :)

This PR: - It adds a few boolean variants of some methods that were missing - It simplifies the implementation of plenty of the operations - Adds ModularIndexing to the SymPy interpreter cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 [ghstack-poisoned]

This PR: - It adds a few boolean variants of some methods that were missing - It simplifies the implementation of plenty of the operations - Adds ModularIndexing to the SymPy interpreter cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 [ghstack-poisoned]

Simplify and extend ValueRanges

4d2fa62

This PR: - It adds a few boolean variants of some methods that were missing - It simplifies the implementation of plenty of the operations - Adds ModularIndexing to the SymPy interpreter [ghstack-poisoned]

lezcano requested a review from eellison July 3, 2023 22:32

lezcano added module: inductor ciflow/inductor release notes: inductor labels Jul 3, 2023

lezcano added 7 commits July 4, 2023 00:42

eellison reviewed Jul 6, 2023

View reviewed changes

torch/utils/_sympy/value_ranges.py Show resolved Hide resolved

torch/utils/_sympy/value_ranges.py Show resolved Hide resolved

eellison approved these changes Jul 6, 2023

View reviewed changes

pytorchbot added the open source label Jul 7, 2023

pytorchmergebot added the Merged label Jul 7, 2023

pytorchmergebot closed this in ff05f81 Jul 7, 2023

facebook-github-bot deleted the gh/Lezcano/202/head branch July 11, 2023 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and extend ValueRanges #104557

Simplify and extend ValueRanges #104557

lezcano commented Jul 3, 2023 •

edited by pytorch-bot bot

pytorch-bot bot commented Jul 3, 2023 •

edited

eellison left a comment

eellison Jul 6, 2023

lezcano Jul 6, 2023

eellison Jul 6, 2023

lezcano Jul 6, 2023

eellison Jul 6, 2023

lezcano Jul 6, 2023

eellison Jul 6, 2023

lezcano Jul 6, 2023

lezcano Jul 6, 2023

lezcano commented Jul 6, 2023

lezcano commented Jul 6, 2023

lezcano commented Jul 6, 2023

Simplify and extend ValueRanges #104557

Simplify and extend ValueRanges #104557

Conversation

lezcano commented Jul 3, 2023 • edited by pytorch-bot bot

pytorch-bot bot commented Jul 3, 2023 • edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/104557

❌ 1 New Failure

eellison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lezcano commented Jul 6, 2023

lezcano commented Jul 6, 2023

lezcano commented Jul 6, 2023

lezcano commented Jul 3, 2023 •

edited by pytorch-bot bot

pytorch-bot bot commented Jul 3, 2023 •

edited