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

Accesses inside indexing expressions should be in compute bounds #6131

Closed
alexreinking opened this issue Jul 8, 2021 · 6 comments · Fixed by #6294
Closed

Accesses inside indexing expressions should be in compute bounds #6131

alexreinking opened this issue Jul 8, 2021 · 6 comments · Fixed by #6294
Assignees

Comments

@alexreinking
Copy link
Member

alexreinking commented Jul 8, 2021

This is a bit of a loose thought, but I'm wondering how bounds inference handles cases like this:

h(x) = 10;
g(x) = sin(x); // whatever
f(x) = g(h(x));

h.compute_root();
g.compute_root();
f.split(x, xo, xi, 8, TailStrategy::RoundUp);

f.realize(10);

It seems to me there might be a hazard with h(x) being uninitialized between x = 10-15, unless accesses inside index expressions are required to be in the compute bounds (so the mem bounds of f become the compute bounds of h) - is that how BI actually works?

I couldn't find a case (at least during POPL paper crunch) to actually test this because the compiler is a bit too smart about inlining.

@abadams
Copy link
Member

abadams commented Jul 9, 2021

This segfaults for me inside the generated code:

#include "Halide.h"

using namespace Halide;

int main(int argc, char **argv) {
    Var x;

    Func f, g, h, out;

    const int min = -10000000;
    const int max = min + 20;

    h(x) = clamp(x, min, max);
    // Within its compute bounds, h's value will be within
    // [min,max]. Outside of that it's uninitialized memory.

    g(x) = sin(x);
    // Halide thinks g will be accessed within [min,max], so its
    // allocation bounds will be [min,max]

    f(x) = g(h(x));
    f.vectorize(x, 64, TailStrategy::RoundUp);
    // f will access h at values outside its compute bounds, and get
    // garbage, and then use that garbage to access g outside of its
    // allocation bounds.

    out(x) = f(x);

    h.compute_root();
    g.compute_root();
    f.compute_root();

    out.realize({1});
    return 0;
}

@abadams
Copy link
Member

abadams commented Jul 9, 2021

g is stack allocated, so I had to make min negative to get it to go to a bad address.

@abadams
Copy link
Member

abadams commented Jul 9, 2021

Proposed fix: When a Func f accesses a Func g using an index which depends on any way in some third Func h, and h is not compute_at within f's produce node, and the schedule for f uses RoundUp or ShiftInwards, and the FuncValueBounds of h are smaller than the bounds of the type of h, then we should either:

  1. Throw an error
  2. Inject a clamp around the call to h to clamp the loaded value to the FuncValueBounds so that FuncValueBounds isn't lying.

@alexreinking
Copy link
Member Author

alexreinking commented Jul 9, 2021

and h is not compute_at within f's produce node

This is because h's compute bounds cover all the points touched in f's loop bounds even though they technically don't have to, right?

@abadams
Copy link
Member

abadams commented Jul 9, 2021

Correct. If in future we managed to give h tighter bounds that than we'd have to relax the condition.

@alexreinking
Copy link
Member Author

If in future we managed to give h tighter bounds that than we'd have to relax the condition.

Cool - it's worth having that documented in our conversation here.

@alexreinking alexreinking self-assigned this Aug 18, 2021
alexreinking added a commit that referenced this issue Oct 7, 2021
Inject clamps around func calls h(...) when all the following conditions hold:
  1. The call is in an indexing context, such as: f(x) = g(h(x));
  2. The schedule for f uses RoundUp or ShiftInwards
  3. h is not compute_at within f's produce node
  4. The FuncValueBounds of h are smaller than those of its type

Conditions (3) and (4) are not yet implemented.
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.
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 a pull request may close this issue.

2 participants