New scheduling directive: compute_with by psuriana · Pull Request #1546 · halide/Halide

psuriana · 2016-10-14T19:44:58Z

This PR added new scheduling directive, compute_with, which fuses iteration over some stage of a function with another stage from outermost loop to a given loop level, e.g. g.compute_with(f, y) is equivalent to:

for y = union of f and g bounds
   for x = ...
      if (y within f bound):
        compute f
   for x = ...
      if (y within g bound):
        compute g

The granularity of compute_with is function definition (stage). The stage on which compute_with is called will be executed AFTER the stage it's computed with in the innermost fused loop. All fused stages have to have the same schedule from outermost to the innermost fused loop dimension.

NOTE: compute_with of update stages of the same Func is now disabled since it is non-trivial to prove that there are no loop-carry dependencies.

steven-johnson · 2016-10-18T23:13:54Z

test/correctness/compute_with.cpp

@@ -0,0 +1,745 @@
+#include "Halide.h"
+#include "../common/check_call_graphs.h"


Please avoid using relative include paths with #include; it makes working with Bazel harder than it could be. Use the pattern we're now using elsewhere:

#include "test/common/check_call_graphs.h"

jingpu · 2017-06-16T18:42:24Z

If a compute_with schedule can fuse an initial definition and an update definition, it looks like it is changing the interleaving granularity of a producer-consumer pair. Would it be better to express this intent using a compute_at?

Maybe I am trying to ask with this PR when to use compute_with versus compute_at.

psuriana · 2017-06-16T20:17:33Z

The idea with compute_with is to allow multiple updates of a Func within the same loop nest (e.g. if you want to schedule the updates within the same GPU loop nest) as long as there are no loop-carried dependencies. You can't do this with compute_at, since it is per Func granularity (each update will be within separate loop nests).

zvookin · 2017-06-20T00:14:00Z

apps/camera_pipe/camera_pipe_generator.cpp

            vec = 64;
        }
-        for (Func f : intermediates) {
+        auto apply_schedule_to_intermediate = [&](Func& f){


Since the method this is in is called "schedule" I think we can name lambdas like this "schedule_intermediate" or perhaps "fold_vectorize_and_place" if a more descriptive name seems useful.

zvookin · 2017-06-20T00:27:54Z

src/BoundsInference.cpp

+                const auto iter = std::find_if(fused_groups[i].begin(), fused_groups[i].end(),
+                    [&producing_func](const Function& f) { return (f.name() == producing_func.name()); });
+                if (iter != fused_groups[i].end()) {
+                    index = i;


This will not pass strict signed/unsigned conversion checks, which I'd like to turn on at some point. I think the clearest way to do this with std algorithms is an outer std::find_if and a std::any_of nested inside.

zvookin

Have not reviewed ScheduleFunctions.cpp yet.

zvookin · 2017-06-20T00:41:13Z

src/BoundsInference.cpp

+            const vector<Dim> &dims = (producing_stage == 0) ? producing_func.definition().schedule().dims() :
+                producing_func.update(producing_stage-1).schedule().dims();
+
+            int var_index;


Should be size_t.

Or possibly ptrdiff_t.

zvookin · 2017-06-20T00:56:53Z

src/BoundsInference.cpp

        funcs[i] = env.find(order[i])->second;
    }

+    vector<vector<Function>> fused_func_groups(fused_groups.size());


This would likely be clearer using range based for loops and putting the values directly into the result vector. (You can use resize or reserve/emplace to control the sizes.)

(I get that this feedback is a bit scratching the surface, but the general gist is that there's a mixture of container programming style going on here, and it's making it a bit tricky to follow the logic. Hence I'm adding some comments as I go.)

zvookin · 2017-06-20T01:03:17Z

src/BoundsInference.cpp

            if (starts_with(op->name, stages[i].stage_prefix)) {
                producing = i;
                f = stages[i].func;
+                stage_index = stages[i].stage;


Cast size_t to int?

zvookin · 2017-06-20T20:52:50Z

src/Func.h

 /** A single definition of a Func. May be a pure or update definition. */
 class Stage {
+    /** Reference to the Function this stage (or definition) belongs to. */
+    Internal::Function func;


Andrew and I would prefer if we didn't abbreviate to "func" for a thing that is a Function (not a Func). Please consider either spelling out "function" or maybe using "fun."

zvookin · 2017-06-20T20:54:44Z

src/Func.h

+    Internal::Function func;
    Internal::Definition definition;
-    std::string stage_name;
+    size_t stage;


Is this field the index in the stage list? Is there a better name.

At some level, it is starting to feel like maybe we want a more expressive data structure, such as an actual graph or perhaps indexable maps. I don't think this is necessary for this PR, but we should think about it a little.

zvookin · 2017-06-21T00:14:04Z

src/Schedule.h

    Auto
 };

+/** Different ways to handle the case when the start/end of the loops of stages


Definitely need more detail, but I think putting examples in the comment on the compute_with directive is the best way to do that. (Can also link to a tutorial I suppose.)

zvookin · 2017-06-21T00:16:05Z

src/Schedule.h

 * recursively injecting realizations for them at particular sites
- * in this loop nest. A LoopLevel identifies such a site. */
+ * in this loop nest. A LoopLevel identifies such a site. The site
+ * can either be a specific loopness within all stages of a function


I'm not sure what "specific loopness" is. Is it meant to be "loop nest" or does this mean "a loop over over the same dimension across all stages of a function"? ("Loop nest" would imply a set of loops all of which are on the same var or dim I think, but it isn't quite clear to me even if that is the wording.)

zvookin · 2017-06-21T00:23:11Z

src/Schedule.h

    bool fold_forward;
 };

+/** This indicates two function stages which loopness are fused from outermost


Perhaps: "This represents two stages with fused loop nests from outermost to a specific loop level. The loops to compute func_1(stage_1) are fused with the loops to compute func_2(stage_2) from outermost to loop level var_name and the computation from stage_1 of func_1 occurs first."

zvookin · 2017-06-21T00:24:27Z

src/Schedule.h

+    /** Until which loop level (starting from outermost) we should fuse
+     * computation of this function stage with another function stage? The
+     * function we are fusing this function with and this function should
+     * be independent of each other. See \ref Func::compute_with and


What does "independent of each other" mean? They're not required to be distinct are they?

zvookin · 2017-06-21T00:25:56Z

src/Schedule.h

    std::vector<PrefetchDirective> &prefetches();
    // @}

+    /** Until which loop level (starting from outermost) we should fuse


"Innermost loop level of fused loop nest for this function stage. Fusion runs from outermost to this loop level." (Definitely remove question mark unless I'm missing something.)

zvookin

A few more.

zvookin · 2017-06-21T19:19:43Z

src/ScheduleFunctions.cpp

 };
+
+bool var_name_match(string v1, string v2) {
+    if (v1 == v2) return true;


I really do not like if statements without braces. (Long conversation, but at the time there was a rash of SSL bugs related to bad else balancing, I decided the language construct is simply indefensible. If one is programming control flow, use the braces. If not, use boolean logic or some other mechanism.) In this case a single return statement and a concatenation of || operators is likely what you want.

zvookin · 2017-06-21T23:52:44Z

src/ScheduleFunctions.cpp

+    // otherwise, this may mess up the bounds_touched computation.
+    int n_predicates_inner = 0;
+    for (int i = start_fuse; (i >= 0) && (i < (int)stage_s.dims().size()-1); ++i) {
+        const Dim &dim = stage_s.dims()[i];


It appears every use of "dim" here is in "prefix + dim.var" so it might be clearer to just make a temporary on that expression.

zvookin · 2017-06-21T23:59:02Z

src/ScheduleFunctions.cpp

+
+    void visit(const Realize *op) {
+        IRVisitor::visit(op);
+        if (op->name == func) result = true;


Prefer "result = result || (op->name == func)"

psuriana · 2017-10-06T22:16:21Z

@abadams PTAL

abadams · 2017-10-06T22:53:35Z

src/RealizationOrder.cpp

+
+// Check to see if there is a loop-carried dependence in a 'func' update
+// definition with the previous definition it's computed with.
+bool has_loop_carried_dependence(const string &func, const vector<Expr> &prev_args,


I'm concerned about this case:

f(x) = x;
RDom r(0, 3);
f(r) += 3;
f(r+1) *= 2;

Fusing on r changes the output

dsharletg

Really nice work on the tests!

dsharletg · 2017-11-03T20:25:56Z

src/Func.h

+     * stage 's' from outermost loop to a given LoopLevel. 'this' stage will
+     * be computed AFTER 's' in the innermost fused dimension. There should not
+     * be any dependencies between those two fused stages. They should not have
+     * extern definitions either. An update stage can be computed with its


Can you clarify the comment regarding extern definitions?

dsharletg · 2017-11-03T20:26:31Z

src/Func.h

+     * be computed AFTER 's' in the innermost fused dimension. There should not
+     * be any dependencies between those two fused stages. They should not have
+     * extern definitions either. An update stage can be computed with its
+     * immediate preceeding stage given that there is no loop-carried


Is the comment regarding update stages still correct given abadams@ recent question and subsequent tweaks?

compute_with of update stages of the same Func is now disabled.

dsharletg · 2017-11-03T21:19:46Z

src/Schedule.h

+    /** Shift the end of the fused loops to align. */
+    AlignEnd,
+
+    /** compute_with will make no attemp to align the start/end of the


steven-johnson · 2017-11-03T22:42:03Z

src/BoundsInference.cpp

            return true;
        }

+        // Determine if the current producing stage are fused with other


steven-johnson · 2017-11-03T23:08:05Z

src/Func.h

+         if (y < size/2-1):
+           g(x, y) = input(2*x, 2*y)
+     \endcode
+     *


steven-johnson · 2017-11-03T23:12:02Z

src/Schedule.cpp

    return contents->func_name;
 }

+int LoopLevel::stage_index() const {


There will be a nontrivial merge collision between this and #2504, but I don't think that it will be hard to resolve.

What is causing the collision?

steven-johnson · 2017-11-03T23:14:07Z

src/Schedule.h

-    EXPORT LoopLevel(Internal::Function f, VarOrRVar v);
-    EXPORT LoopLevel(Func f, VarOrRVar v);
+    EXPORT LoopLevel(Internal::Function f, VarOrRVar v, int stage_level = -1);
+    EXPORT LoopLevel(Func f, VarOrRVar v, int stage_level = -1);


This is a user-facing ctor: should a user ever pass anything other than -1? What happens if they do?

abadams · 2017-11-04T00:32:10Z

src/ScheduleFunctions.cpp

+            DeviceAPI device_api = op->device_api;
+            if (is_one(extent_val)) {
+                for_type = ForType::Serial;
+                device_api = DeviceAPI::None;


Not at all obvious that this is safe. A GPUBlocks loop of extent one is different to a Hexagon loop of extent one is different to a serial loop of extent 1. I assume it's safe because the loops have already been checked for compatibility?

…irective

psuriana · 2017-12-08T18:57:13Z

PTAL. compute_with of stages of the same Func is now disabled. Only fusion of stages with no producer/consumer relationship is allowed.

dsharletg

Just took a quick skim since I've reviewed this once before. I have only minor comments, except for one recurring one: I still see code to support fusing updates from the same func, weren't we going to drop that capability?

dsharletg · 2017-12-08T22:51:54Z

src/RealizationOrder.cpp

+    for (const auto &fn : order) {
+        if (visited.find(fn) == visited.end()) {
+            vector<string> group;
+            find_fused_groups_dfs(fn, fuse_adjacency_list, visited, group);


I think doing this might make the implementation of find_fused_groups_dfs a bit more complicated/less efficient (line 33 would need to append the result of that recursive call to it's own internal copy).

dsharletg · 2017-12-08T22:52:35Z

src/RealizationOrder.cpp

+    for (const auto &iter : fused_pairs_graph) {
+        for (const auto &pair : iter.second) {
+            if (pair.func_1 == pair.func_2) {
+                // compute_with among stages of a function is okay,


Is this old? Didn't we decide this is not OK?

Which internal copy are you referring to?

dsharletg · 2017-12-08T22:53:47Z

src/RealizationOrder.h

+ * the realization order and the fused groups in that order.
+ */
+std::pair<std::vector<std::string>, std::vector<std::vector<std::string>>> realization_order(
+	const std::vector<Function> &output, std::map<std::string, Function> &env);


Identation looks off here.

dsharletg · 2017-12-08T23:01:30Z

src/Schedule.h


+/** Different ways to handle the case when the start/end of the loops of stages
+ * computed with (fused) are not aligned. */
+enum class AlignStrategy {


Maybe we should name this LoopAlignStrategy, because AlignStrategy sounds like something we might want to describe a memory alignment option. It also seems like a more accurate description of what it's doing.

dsharletg · 2017-12-08T23:11:56Z

src/ScheduleFunctions.cpp

+        // computed with are the results of the same application of splits/renames/etc.
+        // Also, if it is a split dimension, verify that it doesn't use ShiftInwards
+        // as tail strategy since this may affect correctness.
+        if (p.func_1 == p.func_2) { // Update and its preceding stage are fused


Weren't we going to remove the ability to fuse stages from the same func?

dsharletg

Very nice work!

…irective

dsharletg · 2017-12-14T07:18:56Z

Looks like a few of the build bot issues are legit (e.g. AlignStrategy -> LoopAlignStrategy in camera_pipe).

dsharletg · 2017-12-18T18:15:26Z

Failures look unrelated and enough build bots passed, merging this!

steven-johnson reviewed Oct 18, 2016

View reviewed changes

psuriana mentioned this pull request Nov 22, 2016

Producing multiple outputs where outputs have different extends for each free variable. #1529

Closed

shoaibkamil mentioned this pull request Mar 24, 2017

Add compute_together() to make Tuple at schedule time. #1943

Closed

psuriana force-pushed the compute_with_directive branch 2 times, most recently from 7b29e10 to 1c391e1 Compare May 2, 2017 16:55

fran6co mentioned this pull request Jun 16, 2017

[REBASE] New scheduling directive: compute_with #2140

Closed

zvookin reviewed Jun 20, 2017

View reviewed changes

zvookin reviewed Jun 21, 2017

View reviewed changes

zvookin reviewed Jun 22, 2017

View reviewed changes

abadams reviewed Oct 6, 2017

View reviewed changes

psuriana force-pushed the compute_with_directive branch from 4d3bc4f to dfe49b5 Compare October 31, 2017 20:38

dsharletg reviewed Nov 3, 2017

View reviewed changes

steven-johnson reviewed Nov 4, 2017

View reviewed changes

abadams reviewed Nov 4, 2017

View reviewed changes

abadams force-pushed the master branch from 1e512fa to 08f2ea7 Compare December 3, 2017 16:38

abadams force-pushed the compute_with_directive branch from 2d3ae5e to 03f2cb5 Compare December 3, 2017 16:53

abadams force-pushed the master branch from 08f2ea7 to 9e23eab Compare December 3, 2017 17:23

abadams force-pushed the compute_with_directive branch 2 times, most recently from 2f84851 to 2d3ae5e Compare December 4, 2017 20:29

psuriana force-pushed the compute_with_directive branch 2 times, most recently from 4e3115f to 959dbcc Compare December 7, 2017 22:40

Add scheduling directive compute_with

6003aa9

psuriana force-pushed the compute_with_directive branch from 3509efc to 6003aa9 Compare December 8, 2017 00:58

psuriana added 2 commits December 8, 2017 10:46

Use IRMutator2 instead of IRMutator

36fccd5

Merge branch 'master' of github.com:halide/Halide into compute_with_d…

7dc24f2

…irective

dsharletg reviewed Dec 8, 2017

View reviewed changes

Remove unused code

c28fd22

dsharletg approved these changes Dec 11, 2017

View reviewed changes

Merge branch 'master' of github.com:halide/Halide into compute_with_d…

2e6dae3

…irective

psuriana and others added 2 commits December 14, 2017 08:11

Fix compilation error in camera pipe

cc8ceb0

Fix python-binding error for Stage::name

ab2d2ff

dsharletg merged commit dabe67e into master Dec 18, 2017

dsharletg deleted the compute_with_directive branch December 20, 2017 00:27

GokulNC mentioned this pull request Oct 26, 2018

Undefined reference to halide_gpu_thread_barrier() #3397

Closed

		@@ -0,0 +1,745 @@
		#include "Halide.h"
		#include "../common/check_call_graphs.h"

Comments

Conversation

psuriana commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jingpu commented Jun 16, 2017

Uh oh!

psuriana commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zvookin Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zvookin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zvookin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psuriana commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsharletg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psuriana commented Dec 8, 2017

Uh oh!

psuriana commented Oct 14, 2016 •

edited

Loading

zvookin Jun 20, 2017 •

edited

Loading