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

Fix scheduling annotations for methods with actions conditional on the arguments #656

Merged
merged 3 commits into from
Dec 30, 2023

Conversation

quark17
Copy link
Collaborator

@quark17 quark17 commented Dec 26, 2023

Fixes #641.

There is some reorganization of the code in ASchedule that constructs the three conflict graphs (CF, SC, and PC, indicating the scheduling limitations between rules/methods). This code contained forced evaluations of the graphs, to prevent memory usage from growing too much (due to the graphs being contructed lazily), and the location of that forcing has moved -- the graphs are constructed in steps (first the initial graph based on pairwise method calls then user pragmas then methods with arguments), and the forcing is now after the first step, not the third, but the first step is the most significant, so experiments suggest that forcing there has the same memory footprint. (It's also probably important to force each graph before starting the next, so all three aren't in memory at once.) If any issues are detected, we can profile the example and revisit this choice.

There was already a step that was adding conflict edges for ActionValue methods where arguments are used in the return value. That just needed to be extended to also check for Action/ActionValue methods where arguments are used in the condition of an action. The effort of both checks is now avoided by first looking for whether a conflict edge already exists for the method relative to itself, and skipping the checks if so. (This was the motivation for the reorganization: so that an initial SC graph was available for lookup when computing the edges to add.)

Test case added.

There was only one group of existing tests affected by the change. It was an example where a method was writing into a vector of registers and the specific register index was coming from an argument:

Vector#(Size, Reg#(Bool))  rgs <- replicateM( mkReg(False) ) ;

method Action set (Index idx);
  rgs[idx] <= True;
endmethod

BSC was inferring SBR for this method, but that would result in incorrect code-generation for a parent module that called the method twice with different argument values: the first call would be ignored and only the register at the second index would be updated. With this PR, BSC infers C by inserting a conflict edge (for the method with itself) in the SC conflict graph.

so that the order of code matches the order of the compuation.  The
initial conflict graphs are computed and then edges are added to those
graphs, but the code to compute the edges was appearing first.
This reorder will allow the addition of code for new edges that needs
to consult the existing graph first (to avoid unnecessary work).
A consequence of this reordering, though, is that the forcing of the
graphs is delayed until all three are constructed.
If an argument is used in the condition of an action, then the method
should be considered to conflict with itself (not be sequenceable).
This also re-instates the forcing of the initial graphs.

Test case added, and update to the expected schedule dump for a series
of tests that were unrelatedly exhibiting the bug.
@quark17
Copy link
Collaborator Author

quark17 commented Dec 26, 2023

I realized that the new code (for computing the method-arg conflict edges) should go in a separate function, to abstract it away and reduce clutter. So I pushed a new commit that does that.

Because we weren't specifying the HLS version for ghcup to install,
but were specifying the GHC version, the GitHub CI suddenly started
failing when a new HLS version became the default and which didn't
support the GHC version we were using because a newer patch version
became available.  To avoid that, we fix the HLS version, so that we
can update the GHC and HLS versions together, on our schedule.
@quark17 quark17 merged commit f00d205 into B-Lang-org:main Dec 30, 2023
33 checks passed
@quark17 quark17 deleted the issue-641-sbr-cond branch December 30, 2023 00:18
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.

Method with internal input-dependent "if" mistakenly scheduled as SBR to self
1 participant