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

ClampUnsafeAccesses is too conservative #6297

Open
alexreinking opened this issue Oct 8, 2021 · 2 comments
Open

ClampUnsafeAccesses is too conservative #6297

alexreinking opened this issue Oct 8, 2021 · 2 comments
Assignees

Comments

@alexreinking
Copy link
Member

alexreinking commented Oct 8, 2021

ClampUnsafeAccesses which is added by #6294 applies clamps to funcs that appear in indexing expressions, even when that func is not over-computed. We're landing the PR with this (sound, but potentially pessimizing) defect because the implementation is not straightforward, but should move to fix it soon.

@alexreinking alexreinking self-assigned this Oct 8, 2021
alexreinking added a commit that referenced this issue Oct 8, 2021
* Add ClampUnsafeAccesses pass. Fixes #6131

Inject clamps around func calls h(...) when all the following conditions hold:
  1. The call flows into an indexing context, such as: `f(x) = g(h(x))` or `let y = h(x) in f(x) = g(y)`
  2. The FuncValueBounds of h are smaller than those of its type
  3. h's allocation bounds might be wider than its compute bounds

Condition (3) is not yet implemented see #6297.
@steven-johnson
Copy link
Contributor

What would be involved in smartening this? A schedule optimization I was experimenting with is currently being bitten by this, and even adding explicit clamp and/or unsafe_promise_clamped doesn't get around the gratuitous-in-my-case min/max-in-the-inner-loop insertion.

@abadams
Copy link
Member

abadams commented Mar 18, 2022

The problem only rears its head when the caller Func is being overcomputed and, probably as a consequence of this, the called Func was overallocated and contains uninitialized values. We could try to check, for each Func, if the storage bounds equal the compute bounds. We could alternatively sniff the schedule for the current stage of the caller Func to see if overcompute is possible. I think that's just a question of whether there are any splits that aren't GuardWithIf. At this point in lowering it might be hard to tell which stage of the Func we're computing though.

One special-but-common-and-easy-to-detect case: It's safe is if the callee is compute_at somewhere inside the caller. In that case the compute bounds of the callee are known to cover all loaded values by the caller, whether or not they're necessary to cover the compute bounds of the caller, because that's how we do compute bounds inference in that case.

So perhaps the simplest change that might help is to only inject the clamp if the realization of the callee is outside the produce node of the caller.

steven-johnson added a commit that referenced this issue Mar 22, 2022
…6654)

* Eliminate some unnecessary clamping in ClampUnsafeAccesses (#6297)

* Update ClampUnsafeAccesses.cpp

* Update ClampUnsafeAccesses.cpp

* Update ClampUnsafeAccesses.cpp
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

No branches or pull requests

3 participants