Fix bounds for sympy expressions #102722

lezcano · 2023-06-01T11:39:18Z

Stack from ghstack (oldest at bottom):

The analysis for SymPy expressions was incorrect as, even though it said
that the assumption was "smoothness" the assumtion was, in fact, that he
formula was monotone in every variable. In other words, it was
assuming that the derivative does not change signs in any variable (!!).

These issues showed up when using this analysis on a larger set of
formulas, where we would get bounds where lower > upper when using
ModularIndexing.

This PR extends the bounds analysis to SymPy expressions. It also
separates the symbols occuring on SymPy expressions into their own
class, as we don't want to use any fallback in this case.

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

The analysis for SymPy expressions was incorrect as, even though it said that the assumption was "smoothness" the assumtion was, in fact, that he formula was monotone in every variable. In other words, it was assuming that the derivative does not change signs in any variable (!!). These issues showed up when using this analysis on a larger set of formulas, where we would get bounds where lower > upper when using ModularIndexing. This PR extends the bounds analysis to SymPy expressions. It also separates the symbols occuring on SymPy expressions into their own class, as we don't want to use any fallback in this case. [ghstack-poisoned]

pytorch-bot · 2023-06-01T11:39:21Z

🔗 Helpful Links

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

📄 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.

❌ 2 New Failures, 1 Unrelated Failure

As of commit 081e80d:

NEW FAILURES - The following jobs have failed:

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

cuda11.8-py3.10-gcc7-sm86 / test (inductor_torchbench, 1, 1, linux.g5.4xlarge.nvidia.gpu, unstable) (gh)

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

lezcano · 2023-06-01T11:41:26Z

torch/_inductor/ir.py

@@ -217,82 +217,6 @@ def may_convert_to_optional(optional_value, value):
    return optional_value if not value and V.graph.cpp_wrapper else value


-class ModularIndexing(sympy.Function):


Moved all these to utils/_symp/functions.py

lezcano · 2023-06-01T11:41:56Z

torch/fx/experimental/symbolic_shapes.py

@@ -844,83 +844,6 @@ def eval(cls, base, divisor):
        else:
            return base / divisor

-class FloorDiv(sympy.Function):


into _sympy/functions.py

The analysis for SymPy expressions was incorrect as, even though it said that the assumption was "smoothness" the assumtion was, in fact, that he formula was monotone in every variable. In other words, it was assuming that the derivative does not change signs in any variable (!!). These issues showed up when using this analysis on a larger set of formulas, where we would get bounds where lower > upper when using ModularIndexing. This PR extends the bounds analysis to SymPy expressions. It also separates the symbols occuring on SymPy expressions into their own class, as we don't want to use any fallback in this case. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 [ghstack-poisoned]

ezyang

But please see comments about div/floordiv confusion

ezyang · 2023-06-01T12:38:03Z

torch/_inductor/lowering.py

-        phend = ops.index_expr(ir.FloorDiv(h, stride[0]) + 1, torch.int32)
-        pwend = ops.index_expr(ir.FloorDiv(w, stride[1]) + 1, torch.int32)
+        phend = ops.index_expr(FloorDiv(h, stride[0]) + 1, torch.int32)
+        pwend = ops.index_expr(FloorDiv(w, stride[1]) + 1, torch.int32)


With this many updated call sites it would have been better to do this refactor in a separate PR (or not do it? This seems like window dressing to me for not much benefit)

I just needed to move all the relevant special classes into one folder so that I can import them properly in the other parts of the analysis. All the changers in this file just remove the ir. from the import.
In all fairness, I do think it's nicer without the ir., as this is not an IR class, but a SymPy construct.

ezyang · 2023-06-02T13:05:49Z

torch/_inductor/optimize_indexing.py

@@ -63,74 +62,6 @@ def range_expressable_in_32_bits(range):
    )


-def get_expr_range(expr, vars_ranges: dict):


Did this get moved?

This one was incorrect and it's now replaced by bound_sympy.

torch/utils/_sympy/value_ranges.py

ezyang · 2023-06-02T13:12:34Z

torch/utils/_sympy/interp.py

@@ -37,7 +40,7 @@ def handlers():
        sympy.Ge: "ge",
        sympy.Not: "not_",
        TrueDiv: "truediv",
-        FloorDiv: "div",
+        FloorDiv: "floordiv",


Is this OK? I was under the impression the analysis was also used by Inductor, which has its own naming convention for operation methods we need to abide by.

I think this is the right one, yes. See #102722 (comment)

ezyang · 2023-06-02T13:18:07Z

torch/utils/_sympy/value_ranges.py

-        return index
+    @staticmethod
+    def not_(a):
+        return ValueRanges.decreasing_map(a, sympy.Not)


I would greatly appreciate it if these helper functions had mathematical definitions with them.

In the case of decreasing map, f is decreasing if when `x < y, then f(y) < f(x)

Sure, I can do that.

ezyang · 2023-06-02T13:21:24Z

torch/utils/_sympy/value_ranges.py

-        return ValueRanges.decreasing_map(x, operator.neg)
+    @classmethod
+    def div(cls, a, b):
+        return cls.truediv(a, b)


Once again, I'm not sure this is kosher because it is inconsistent with Inductor cpp codegen

@staticmethod def div(a, b): return f"{a} / {b}"

from codegen.cpp

Note that this change was not introduced in this PR, but in a previous one. The relevant discussion is in #100547 (comment)

ezyang · 2023-06-02T13:27:38Z

torch/utils/_sympy/value_ranges.py

+        x = ValueRanges.wrap(x)
+        if x.lower < 0:
+            return ValueRanges.unknown()
+        return ValueRanges.increasing_map(x, sympy.sqrt)


Which of these changed, which are the same?

Yeah, the diff is just terrible. Let me split the "moving things around" and the "improve things" into a few PRs

The analysis for SymPy expressions was incorrect as, even though it said that the assumption was "smoothness" the assumtion was, in fact, that he formula was monotone in every variable. In other words, it was assuming that the derivative does not change signs in any variable (!!). These issues showed up when using this analysis on a larger set of formulas, where we would get bounds where lower > upper when using ModularIndexing. This PR extends the bounds analysis to SymPy expressions. It also separates the symbols occuring on SymPy expressions into their own class, as we don't want to use any fallback in this case. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 [ghstack-poisoned]

The analysis for SymPy expressions was incorrect as, even though it said that the assumption was "smoothness" the assumtion was, in fact, that he formula was monotone in every variable. In other words, it was assuming that the derivative does not change signs in any variable (!!). These issues showed up when using this analysis on a larger set of formulas, where we would get bounds where lower > upper when using ModularIndexing. This PR extends the bounds analysis to SymPy expressions. It also separates the symbols occuring on SymPy expressions into their own class, as we don't want to use any fallback in this case. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 [ghstack-poisoned]

lezcano · 2023-07-03T22:30:41Z

split into #104556 #104557 #104559

The analysis for SymPy expressions was incorrect as, even though it said that the assumption was "smoothness" the assumtion was, in fact, that he formula was monotone in every variable. In other words, it was assuming that the derivative does not change signs in any variable (!!). These issues showed up when using this analysis on a larger set of formulas, where we would get bounds where lower > upper when using ModularIndexing. This PR extends the bounds analysis to SymPy expressions. It also separates the symbols occuring on SymPy expressions into their own class, as we don't want to use any fallback in this case. ghstack-source-id: ac14eedada2039f7753251092c0e7a70765ed4ab Pull Request resolved: pytorch/pytorch#102722

lezcano mentioned this pull request Jun 1, 2023

Refactor OptimizeIndexing #100549

Closed

pytorch-bot bot added the release notes: fx release notes category label Jun 1, 2023

lezcano mentioned this pull request Jun 1, 2023

ValueRange analysis for indirect indexing #102611

Closed

github-actions bot added ciflow/inductor module: inductor labels Jun 1, 2023

lezcano commented Jun 1, 2023

View reviewed changes

pytorchbot added the open source label Jun 1, 2023

lezcano added the suppress-bc-linter Suppresses the failures of API backward-compatibility linter (Lint/bc_linter) label Jun 1, 2023

lezcano added 2 commits June 1, 2023 12:28

lezcano requested review from ezyang and eellison June 1, 2023 12:29

lezcano added the release notes: inductor label Jun 1, 2023

lezcano removed the release notes: fx release notes category label Jun 1, 2023

lezcano added 5 commits June 1, 2023 13:55

ezyang approved these changes Jun 2, 2023

View reviewed changes

lezcano added 6 commits June 2, 2023 14:45

lezcano added the with-ssh label Jun 3, 2023

ngimel mentioned this pull request Jun 9, 2023

indutor: fix issue of compute index_expr range #103147

Closed

lezcano mentioned this pull request Jul 1, 2023

linker hack #104498

Closed

lezcano closed this Jul 3, 2023

facebook-github-bot deleted the gh/Lezcano/199/head branch August 3, 2023 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bounds for sympy expressions #102722

Fix bounds for sympy expressions #102722

lezcano commented Jun 1, 2023 •

edited

pytorch-bot bot commented Jun 1, 2023 •

edited

lezcano Jun 1, 2023

lezcano Jun 1, 2023

ezyang left a comment

ezyang Jun 1, 2023

lezcano Jun 2, 2023

ezyang Jun 2, 2023

lezcano Jun 2, 2023

ezyang Jun 2, 2023

lezcano Jun 2, 2023

ezyang Jun 2, 2023

lezcano Jun 2, 2023

ezyang Jun 2, 2023

lezcano Jun 2, 2023

ezyang Jun 2, 2023

lezcano Jun 2, 2023

lezcano commented Jul 3, 2023

		@@ -217,82 +217,6 @@ def may_convert_to_optional(optional_value, value):
		return optional_value if not value and V.graph.cpp_wrapper else value


		class ModularIndexing(sympy.Function):

		@@ -63,74 +62,6 @@ def range_expressable_in_32_bits(range):
		)


		def get_expr_range(expr, vars_ranges: dict):

Fix bounds for sympy expressions #102722

Fix bounds for sympy expressions #102722

Conversation

lezcano commented Jun 1, 2023 • edited

pytorch-bot bot commented Jun 1, 2023 • edited

🔗 Helpful Links

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

❌ 2 New Failures, 1 Unrelated Failure

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezyang 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lezcano commented Jul 3, 2023

lezcano commented Jun 1, 2023 •

edited

pytorch-bot bot commented Jun 1, 2023 •

edited