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

Bound allocation extents for hoist_storage using loop variables one-by-one #8154

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

vksnk
Copy link
Member

@vksnk vksnk commented Mar 13, 2024

This is a fix for #8141.

As described there, the problem is that bound_of_expr_in_scope is not recursive: when the storage is hoisted, the allocation extents may depend on the loop variables, so we find a maximum bound for them using bound_of_expr_in_scope and the scope which has intervals for each of the loop variables. However, the loop variables themselves may depend on the other loop variables in which case the extent still may refer to loop variables even after calling bound_of_expr_in_scope.

The fix is to apply bound_of_expr_in_scope for each loop variable separately in a reverse order. I wasn't sure if there is a more efficient way to achieve this, but hopefully it isn't so bad, because allocation extents are usually pretty small.

@vksnk vksnk requested a review from abadams March 13, 2024 18:12
@abadams
Copy link
Member

abadams commented Mar 13, 2024

A possibly-easier option might be to do it on the way in, in the for visitor:

    Stmt visit(const For *op) override {
        Expr expanded_min = op->min;
        Expr expanded_extent = op->extent;
        // Iterate from innermost outwards
        for (auto it = hoisted_storages.rbegin(); it != hoisted_storages.rend(); it++) {
            expanded_min = simplify(expand_expr(expanded_min, it->scope));
            expanded_extent = expand_expr(expanded_extent, it->scope);
            Interval loop_bounds = Interval(expanded_min, simplify(expanded_min + expanded_extent - 1));
            loop_bounds.min = bounds_of_expr_in_scope(loop_bounds.min, it->loop_vars).min;
            loop_bounds.max = bounds_of_expr_in_scope(loop_bounds.max, it->loop_vars).max;
            it->loop_vars.push(op->name, loop_bounds);
        }

@vksnk
Copy link
Member Author

vksnk commented Mar 14, 2024

A possibly-easier option might be to do it on the way in, in the for visitor:

    Stmt visit(const For *op) override {
        Expr expanded_min = op->min;
        Expr expanded_extent = op->extent;
        // Iterate from innermost outwards
        for (auto it = hoisted_storages.rbegin(); it != hoisted_storages.rend(); it++) {
            expanded_min = simplify(expand_expr(expanded_min, it->scope));
            expanded_extent = expand_expr(expanded_extent, it->scope);
            Interval loop_bounds = Interval(expanded_min, simplify(expanded_min + expanded_extent - 1));
            loop_bounds.min = bounds_of_expr_in_scope(loop_bounds.min, it->loop_vars).min;
            loop_bounds.max = bounds_of_expr_in_scope(loop_bounds.max, it->loop_vars).max;
            it->loop_vars.push(op->name, loop_bounds);
        }

Thanks! They seem to be roughly equivalent w.r.t to number of times bounds_of_expr_in_scope has to be called, so I'll just keep it as is.

@vksnk vksnk merged commit f841a27 into main Mar 14, 2024
19 checks passed
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 this pull request may close these issues.

2 participants