loopnode: entry/exit/backedge var API

[caleridas]

Add API grouping vars as EntryVar/ExitVar/BackEdgeVar. Provide functions similar to theta, gamma etc to deal with these variables.

Use this API in DNE, and simplify how they operate.

This does not yet consistently use the new API or
eliminate the subclasses of RegionArgument/RegionResult, but is a necessary step.

[phate]

std::logic_error(), no?

[phate]

same here

[caleridas]

@sjalander I need help with this one, apparently this changes cycle times. The change should be almost no-op, except that I guess it may changed ordering of inputs/outputs/arguments. However I might be overlooking something fundamental.

[phate]

@caleridas In order to make the HLS test suite pass, you might need to adjust the golden cycles. You can do this by finding the respective file under .github/golden. For example:

The execution time of base/test_store_loop has changed
    Golden cycle time: 17
    Simulated cycles: 18

you would need to adjust the file .github/golden/hls-test-suite/base/test_store_loop with the new value.

[sjalander]

@caleridas @phate
I'm trying to find out why there is such a big difference. I've managed to find that addressQueueInsertion, for some reason, causes the result to go from 267 nodes to 486 (master vs. loopnode-hl-api).

For updating the golden cycles, there is an argument to the run script to do this automatically.
./scripts/run-hls-test.sh --update-golden

I will try to find out more about why this happens.

[sjalander]

@caleridas

I've not had time to verify this, but as Nico and I are away for the weekend, I thought I provide what has been found as the likely reason for the changed behavior.

Why the LoopNode Branch Does NOT Remove Nodes That Master Removes

The question: given that graph-28 is structurally identical between branches, why does the loopnode branch produce a larger graph after memoryStateDecoupling? The answer lies entirely in one function: remove_loop_passthrough() in rhls-dne.cpp, which has been refactored to silently skip BackEdgeVar passthroughs.

The sole cause: remove_loop_passthrough silently skips BackEdgeVar

What the new code does (rhls-dne.cpp, loopnode branch):

for (auto exitvar : ln->getExitVars()) {
    auto loopval = exitvar.inner->origin();            // value in subregion feeding this output
    if (rvsdg::TryGetRegionParentNode<LoopNode>(*loopval) == ln) {
        auto loopvar = ln->mapArgument(*loopval);      // classify: EntryVar or BackEdgeVar
        if (auto entry = std::get_if<LoopNode::EntryVar>(&loopvar)) {
            // Only reaches here for EntryVar → divert users and remove passthrough
            exitvar.output->divert_users(entry->input->origin());
            any_changed = true;
        }
        // BackEdgeVar → std::get_if<EntryVar> returns nullptr → silently skipped, NOT removed
    }
}

What master's code does: iterates all loop inputs and subregion arguments without type discrimination. If an argument has exactly one user that is a RegionResult, the passthrough is removed regardless of whether the argument is an EntryArgument or BackEdgeArgument.

[caleridas]

@caleridas

I've not had time to verify this, but as Nico and I are away for the weekend, I thought I provide what has been found as the likely reason for the changed behavior.

Why the LoopNode Branch Does NOT Remove Nodes That Master Removes

The question: given that graph-28 is structurally identical between branches, why does the loopnode branch produce a larger graph after memoryStateDecoupling? The answer lies entirely in one function: remove_loop_passthrough() in rhls-dne.cpp, which has been refactored to silently skip BackEdgeVar passthroughs.

The sole cause: remove_loop_passthrough silently skips BackEdgeVar

What the new code does (rhls-dne.cpp, loopnode branch):

for (auto exitvar : ln->getExitVars()) {
    auto loopval = exitvar.inner->origin();            // value in subregion feeding this output
    if (rvsdg::TryGetRegionParentNode<LoopNode>(*loopval) == ln) {
        auto loopvar = ln->mapArgument(*loopval);      // classify: EntryVar or BackEdgeVar
        if (auto entry = std::get_if<LoopNode::EntryVar>(&loopvar)) {
            // Only reaches here for EntryVar → divert users and remove passthrough
            exitvar.output->divert_users(entry->input->origin());
            any_changed = true;
        }
        // BackEdgeVar → std::get_if<EntryVar> returns nullptr → silently skipped, NOT removed
    }
}

What master's code does: iterates all loop inputs and subregion arguments without type discrimination. If an argument has exactly one user that is a RegionResult, the passthrough is removed regardless of whether the argument is an EntryArgument or BackEdgeArgument.

By this description I don't fully understand if/how the original transformation could be sound. AFAICT, there are the following possible cases for a region argument to connect directly to a region result:

• backedge -> backedge: passthrough, can route around; can be removed internally if not used otherwise
• entry -> backedge: somewhat strange case, it means the backedge has constant value? in this case could replace all backedge uses with the entry, but it does not cause anything WRT to passthrough
• entry -> result: fairly clear, and this case is handled correctly I think
• backedge -> result: TBH I think this must not lead to removal: the backedge may represent a different value on each iteration, so there is no opportunity for passthrough removal

I am really not clear what the original code does in terms of soundness. As I see it:

• backedge arg/result always comes in pair; it may either be removed entirely or not at all; since it does not input / output, it cannot affect passthrough removal directly
• outputs: they can possibly be removed individually; any output that is directly connect to an input can be short-circuited (passthrough removal), afterwards the output might also be removed as it is now unused
• inputs: they can be removed if unused

so what ought to work for passthrough is:

• check output, whether it forwards an input -> short-circuit the input; now output is unused
• since output is unused, it can be removed
• now the input that the output was forwarding to is also unused, so it can be removed in turn

This is what the refactored code is intending to do, what exactly am I missing? Is there no unit test validating that all inputs / outputs / passthroughs are removed as intended?

[sjalander]

The HLS backend is unfortunately not unit tested, it is something being worked on.

Analysis of backedge -> result -- A Valid Passthrough

Why this is actually a valid passthrough to remove

The key insight is that we are checking whether the BackEdgeResult itself (not its downstream computation) feeds an ExitResult. A BackEdgeResult's pre field holds the pre-iteration value of the loop-carried dependency -- which is the correct value for this iteration from the outside loop's perspective. When an ExitResult directly forwards this BackEdgeResult with no intermediate computation:

1. The external consumer wants "the value on each iteration"
2. That value is exactly what backedge->pre represents (the value carried into this iteration)
3. Diverting users to backedge->pre gives them the same data, just at a structurally different point in the schedule

The nusers()==1 guard is critical here

We only perform this diversion when the BackEdgeResult has exactly one user (this exit result). If other nodes inside the loop read the backedge, they are not affected by diverting the exit output -- those internal reads remain intact. Only external consumers lose their path, and they are replaced with a path to backedge->pre, which carries the same data.

The following patch to DNE results in identical cycle counts for at least one test case that otherwise has a significant difference:
DNE.patch

[caleridas]

The HLS backend is unfortunately not unit tested, it is something being worked on.

Analysis of backedge -> result -- A Valid Passthrough

Why this is actually a valid passthrough to remove

The key insight is that we are checking whether the BackEdgeResult itself (not its downstream computation) feeds an ExitResult. A BackEdgeResult's pre field holds the pre-iteration value of the loop-carried dependency -- which is the correct value for this iteration from the outside loop's perspective. When an ExitResult directly forwards this BackEdgeResult with no intermediate computation:

1. The external consumer wants "the value on each iteration"
2. That value is exactly what backedge->pre represents (the value carried into this iteration)
3. Diverting users to backedge->pre gives them the same data, just at a structurally different point in the schedule

The nusers()==1 guard is critical here

We only perform this diversion when the BackEdgeResult has exactly one user (this exit result). If other nodes inside the loop read the backedge, they are not affected by diverting the exit output -- those internal reads remain intact. Only external consumers lose their path, and they are replaced with a path to backedge->pre, which carries the same data.

The following patch to DNE results in identical cycle counts for at least one test case that otherwise has a significant difference: DNE.patch

what does "obtaining the value of every loop iteration" mean? I thought there is exactly one value per output, at end of loop -- and not one value generated per each loop iteration (I have difficulty making sense what this ought to mean in RVSDG even).

Also with the proposed patch I do not understand this bit:

       if (auto entry = std::get_if<LoopNode::EntryVar>(&loopvar))
       {
-        exitvar.output->divert_users(entry->input->origin());
-        any_changed = true;
+        // Only divert if the entry arg has exactly one user (this exit result)
+        if (entry->inner->nusers() == 1) {
+          exitvar.output->divert_users(entry->input->origin());
+          any_changed = true;
+        }
+      }
+      else if (auto backedge = std::get_if<LoopNode::BackEdgeVar>(&loopvar))
+      {
+        // Only divert if the backedge arg has exactly one user (this exit result)
+        if (backedge->pre->nusers() == 1) {
+          exitvar.output->divert_users(backedge->pre);
+          any_changed = true;
+        }

the second divert_users to backedge->pre IMHO does not make sense -- backedge->pre is inside the region, but this diverts an output of the loop node to an edge inside the region? how can that work?

lastly, the remove_if condition in the patch appears inverted:

           [](const LoopNode::BackEdgeVar & var)
           {
-            return !(var.pre->nusers() == 1 && var.post->origin() == var.pre);
+            return (var.pre->nusers() == 0 || (var.pre->nusers() == 1 && var.post->origin() == var.pre));
           }),
       vars.end());

Since we are building the list of vars to be removed, we want to remove from the list the variables that should survive -- but this makes exactly all unused variables survive? How can this work?

[sjalander]

One of the cases with a significant change in cycles is decoupled/test_multi_sum_decouple.c.
As the name indicates, this is a decoupled access-and-execute example, where a loop is used to generate a set of addresses that are sent to the memory system; for each iteration, a new address is created and sent off, i.e., the output is not produced only at the termination of the loop.
A second loop is then used to fetch each response and operate on it.

The code of the HLS kernel:

extern void hls_decouple_request_TYPE(uint32_t channel, const TYPE * addr);
extern TYPE hls_decouple_response_TYPE(uint32_t channel, uint32_t buffer_slots);
enum decoupled_channels{
    sum_dec_channel
};
TYPE kernel(TYPE*  a, uint32_t cnt){
    TYPE result = 0;
    for (uint32_t k = 0; k < ITERATIONS; ++k) {
        for (uint32_t j = 0; j < cnt; j++) {
            hls_decouple_request_TYPE(sum_dec_channel, &a[j]);
        }
        for (uint32_t j = 0; j < cnt; j++) {
            result += hls_decouple_response_TYPE(sum_dec_channel, LATENCY);
        }
    }
        return result;
}

The hls_decouple_request_TYPE and hls_decouple_response_TYPE are a naming convention used by the HLS backend to indicate that these are part of DAE. This was done instead of trying to add support for pragmas or similar.