Fix another item memo loss location + bool specialization bug #139587

bobrenjc93 · 2024-11-04T00:23:11Z

Stack from ghstack (oldest at bottom):

This fix was a bit more involved:

It fixes a item_memo loss place.
It updates a test to be eager instead of aot_eager since it reveals a very obscure bug related to replacements that's not worth solving since in practice inductor will regenerate the runtime asserts anyways
It updates tensorify to specialize more places now that the aforementioned bug is fixed.

Fixes PYTORCH_OPINFO_SAMPLE_INPUT_INDEX=6 python test/inductor/test_torchinductor_opinfo.py TestInductorOpInfoCPU.test_comprehensive_linalg_norm_cpu_float16 when specialize_float=False

while ensuring python test/dynamo/test_dynamic_shapes.py DynamicShapesMiscTests.test_runtime_assert_replacement_dynamic_shapes doesn't regress

cc @ezyang @SherlockNoMad @EikanWang @jgong5 @wenzhe-nrv @voznesenskym @penguinwu @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov

[ghstack-poisoned]

pytorch-bot · 2024-11-04T00:23:14Z

🔗 Helpful Links

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

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

✅ No Failures

As of commit 73bdc16 with merge base e6ff07f ():
💚 Looks good so far! There are no failures yet. 💚

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

Fixes `PYTORCH_OPINFO_SAMPLE_INPUT_INDEX=6 python test/inductor/test_torchinductor_opinfo.py TestInductorOpInfoCPU.test_comprehensive_linalg_norm_cpu_float16` when `specialize_float=False` [ghstack-poisoned]

Fixes `PYTORCH_OPINFO_SAMPLE_INPUT_INDEX=6 python test/inductor/test_torchinductor_opinfo.py TestInductorOpInfoCPU.test_comprehensive_linalg_norm_cpu_float16` when `specialize_float=False` cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

Fixes `PYTORCH_OPINFO_SAMPLE_INPUT_INDEX=6 python test/inductor/test_torchinductor_opinfo.py TestInductorOpInfoCPU.test_comprehensive_linalg_norm_cpu_float16` when `specialize_float=False` while ensuring `python test/dynamo/test_dynamic_shapes.py DynamicShapesMiscTests.test_runtime_assert_replacement_dynamic_shapes` doesn't regress cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

This fix was a bit more involved: 1) It fixes a item_memo loss place. 2) It fixes a bug where we would specialize bools without guarding correctly. 3) It updates tensorify to specialize more places now that the aforementioned bug is fixed. Fixes `PYTORCH_OPINFO_SAMPLE_INPUT_INDEX=6 python test/inductor/test_torchinductor_opinfo.py TestInductorOpInfoCPU.test_comprehensive_linalg_norm_cpu_float16` when `specialize_float=False` while ensuring `python test/dynamo/test_dynamic_shapes.py DynamicShapesMiscTests.test_runtime_assert_replacement_dynamic_shapes` doesn't regress cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

ezyang · 2024-11-04T18:11:35Z

torch/fx/experimental/symbolic_shapes.py

+            # would result in replacements[u0] = 3, which would end
+            # result in guards not being added during specialization.
+            # See https://github.com/pytorch/pytorch/pull/138868#discussion_r1823076611
+            # for more information.


Do we need to talk about this more? Isn't it better to just fix this problem?

Yes I think it'd be good to sync on this problem. Here's my perspective:

These constant replacements seem unsound. Let's take a look at this test case:

@torch._dynamo.config.patch(capture_scalar_outputs=True) def test_runtime_assert_replacement(self): @torch.compile(backend="aot_eager") def fn(x, y): z = y.item() torch._check(z == 3) return x + z fn(torch.randn(4), torch.tensor([3])) self.assertRaises(RuntimeError, lambda: fn(torch.randn(4), torch.tensor([4])))

The torch._check(z == 3) results in calling defer_runtime_assert on Eq(u0, 3)

which subsequently calls self._maybe_guard_rel with Eq(u0, 3)

which subsequently calls self._set_replacement(lhs, self._find(rhs), "trivial_lhs") where lhs is u0 and rhs is 3

without early returning if the rhs has no free symbols, we will end up setting a replacement of u0 => 3 WITHOUT adding a guard

This makes it such that the SymBool Eq(u0, 3) gets simplified to Eq(3, 3) which gets simplified to True

Now when we get to our tensorify pass and we specialize

if isinstance( (val := node.meta.get("val")), (torch.SymFloat, torch.SymInt, torch.SymBool), ): if all( symbol_is_type(s, SymT.FLOAT) for s in val.node.expr.free_symbols ): # If all symbols are backed symfloats, we can just specialize the whole node # and get more precise guards. eg. # # zf = a.item() # zf2 = zf // 2 # op(.. zf2 ..) # # It's better to guard on zf // 2 == 2.0 than zf == 5.0 node.replace_all_uses_with(guard_scalar(val)) graph.erase_node(node)

We don't actually end up guarding on u0 == 3 as we expect since the eq SymBool meta val is simply True without any free symbols

So, I'm not exactly sure what the bug fix is, but let me describe how it should work.

We must distinguish between two cases for output value of item(). If item() returns an unbacked SymInt, it can never be truly "eliminated". Even if we setup a replacement (u0 == 3), we still know that this item() is the binding site for u0 (via unbacked_bindings) and we are obligated to generate a replacement assert for it when u0 comes into scope. Now, I could believe this is not currently done. Here is where assertions are codegenned

def make_assert(expr: SympyBoolean, msg: str) -> None: assert_op = ir.AssertScalar(expr, msg) self.register_buffer(assert_op, set_name=True) self.register_operation(assert_op) for i0 in new_unbacked_defs: ras = self.ras_by_symbol.pop(i0, []) # NB: size-like not needed, we won't retrace vr = shape_env.var_to_range[i0] if not shape_env._default_unspecified_value_range().issubset(vr): def is_convertible(s: Expr) -> bool: if s in (int_oo, -int_oo): return False try: int(s) return True except TypeError: return False if is_convertible(vr.lower): make_assert(i0 >= vr.lower, f"{i0} >= {vr.lower}") if is_convertible(vr.upper): make_assert(i0 <= vr.upper, f"{i0} <= {vr.upper}") for ra in ras: fvs = free_unbacked_symbols(ra.expr) missing = fvs - self.bound_unbacked_symbols if missing: i1 = min(missing, key=str) self.ras_by_symbol.setdefault(i1, []).append(ra) else: make_assert(ra.expr, f"{ra.expr}")

I don't see handling for replacements. So it seems plausible this is just an oopsie that needs to be fixed here

…bug" This fix was a bit more involved: 1) It fixes a item_memo loss place. 2) It fixes a bug where we would specialize bools without guarding correctly. 3) It updates tensorify to specialize more places now that the aforementioned bug is fixed. Fixes `PYTORCH_OPINFO_SAMPLE_INPUT_INDEX=6 python test/inductor/test_torchinductor_opinfo.py TestInductorOpInfoCPU.test_comprehensive_linalg_norm_cpu_float16` when `specialize_float=False` while ensuring `python test/dynamo/test_dynamic_shapes.py DynamicShapesMiscTests.test_runtime_assert_replacement_dynamic_shapes` doesn't regress cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

…bug" This fix was a bit more involved: 1) It fixes a item_memo loss place. 2) It updates a test to be eager instead of aot_eager since it reveals a very obscure bug related to replacements that's not worth solving since in practice inductor will regenerate the runtime asserts anyways 3) It updates tensorify to specialize more places now that the aforementioned bug is fixed. Fixes `PYTORCH_OPINFO_SAMPLE_INPUT_INDEX=6 python test/inductor/test_torchinductor_opinfo.py TestInductorOpInfoCPU.test_comprehensive_linalg_norm_cpu_float16` when `specialize_float=False` while ensuring `python test/dynamo/test_dynamic_shapes.py DynamicShapesMiscTests.test_runtime_assert_replacement_dynamic_shapes` doesn't regress cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

ezyang

bwahaha, lol

bobrenjc93 · 2024-11-09T03:03:48Z

@pytorchbot merge

pytorchmergebot · 2024-11-09T03:05:33Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Fixes `PYTORCH_TEST_WITH_INDUCTOR=1 tlp python test/test_torch.py TestTorchDeviceTypeCUDA.test_cauchy_cuda_float64` when specialize_float=False Pull Request resolved: #139583 Approved by: https://github.com/ezyang ghstack dependencies: #139569, #139457, #139568, #139572, #139846, #139454, #139896, #139935, #139587

…h#139587) This fix was a bit more involved: 1) It fixes a item_memo loss place. 2) It updates a test to be eager instead of aot_eager since it reveals a very obscure bug related to replacements that's not worth solving since in practice inductor will regenerate the runtime asserts anyways 3) It updates tensorify to specialize more places now that the aforementioned bug is fixed. Fixes `PYTORCH_OPINFO_SAMPLE_INPUT_INDEX=6 python test/inductor/test_torchinductor_opinfo.py TestInductorOpInfoCPU.test_comprehensive_linalg_norm_cpu_float16` when `specialize_float=False` while ensuring `python test/dynamo/test_dynamic_shapes.py DynamicShapesMiscTests.test_runtime_assert_replacement_dynamic_shapes` doesn't regress Pull Request resolved: pytorch#139587 Approved by: https://github.com/ezyang ghstack dependencies: pytorch#139569, pytorch#139457, pytorch#139568, pytorch#139572, pytorch#139846, pytorch#139454, pytorch#139896, pytorch#139935

Fixes `PYTORCH_TEST_WITH_INDUCTOR=1 tlp python test/test_torch.py TestTorchDeviceTypeCUDA.test_cauchy_cuda_float64` when specialize_float=False Pull Request resolved: pytorch#139583 Approved by: https://github.com/ezyang ghstack dependencies: pytorch#139569, pytorch#139457, pytorch#139568, pytorch#139572, pytorch#139846, pytorch#139454, pytorch#139896, pytorch#139935, pytorch#139587

Specialize symfloats irrespective of free symbols

d7d0681

[ghstack-poisoned]

This was referenced Nov 4, 2024

support symfloats in translation validation #139457

Closed

Trigger symfloat specialization in argument binding code #139454

Closed

bobrenjc93 mentioned this pull request Nov 4, 2024

don't run z3 analysis on backed symfloat nodes #139568

Closed

pytorch-bot bot added ciflow/inductor release notes: fx release notes category labels Nov 4, 2024

facebook-github-bot added the fx label Nov 4, 2024

Update on "Specialize symfloats irrespective of free symbols"

8cf5b1b

Fixes `PYTORCH_OPINFO_SAMPLE_INPUT_INDEX=6 python test/inductor/test_torchinductor_opinfo.py TestInductorOpInfoCPU.test_comprehensive_linalg_norm_cpu_float16` when `specialize_float=False` [ghstack-poisoned]

pytorch-bot bot added the module: dynamo label Nov 4, 2024

bobrenjc93 added 4 commits November 3, 2024 19:17

ezyang reviewed Nov 4, 2024

View reviewed changes

bobrenjc93 added 4 commits November 6, 2024 15:22

bobrenjc93 added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 8, 2024

bobrenjc93 marked this pull request as ready for review November 8, 2024 04:32

bobrenjc93 requested a review from bdhirsh as a code owner November 8, 2024 04:32

bobrenjc93 requested a review from ezyang November 8, 2024 04:32

pytorch-bot bot added the module: inductor label Nov 8, 2024

ezyang approved these changes Nov 9, 2024

View reviewed changes

pytorchmergebot added the merging label Nov 9, 2024

pytorchmergebot added the Merged label Nov 9, 2024

pytorchmergebot closed this in 4488e23 Nov 9, 2024

pytorchmergebot removed the merging label Nov 9, 2024

github-actions bot deleted the gh/bobrenjc93/110/head branch December 10, 2024 02:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix another item memo loss location + bool specialization bug #139587

Fix another item memo loss location + bool specialization bug #139587

Uh oh!

bobrenjc93 commented Nov 4, 2024 •

edited by pytorch-bot bot

Loading

Uh oh!

pytorch-bot bot commented Nov 4, 2024 •

edited

Loading

Uh oh!

ezyang Nov 4, 2024

Uh oh!

bobrenjc93 Nov 6, 2024

Uh oh!

ezyang Nov 7, 2024

Uh oh!

ezyang left a comment

Uh oh!

bobrenjc93 commented Nov 9, 2024

Uh oh!

pytorchmergebot commented Nov 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix another item memo loss location + bool specialization bug #139587

Fix another item memo loss location + bool specialization bug #139587

Uh oh!

Conversation

bobrenjc93 commented Nov 4, 2024 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ No Failures

Uh oh!

ezyang Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

bobrenjc93 Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

ezyang Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

bobrenjc93 commented Nov 9, 2024

Uh oh!

pytorchmergebot commented Nov 9, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bobrenjc93 commented Nov 4, 2024 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Nov 4, 2024 •

edited

Loading