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

Track data dependencies in SOACs #2006

Merged
merged 55 commits into from
Oct 2, 2023
Merged

Conversation

nhey
Copy link
Member

@nhey nhey commented Aug 9, 2023

No description provided.

nhey added 3 commits August 9, 2023 10:41
map test correctly reduces the number of binary operations,
while scan test does not. Still the dependencies produced by
`opDependencies` are very similar.
@athas
Copy link
Member

athas commented Aug 9, 2023

I have a suggestion. Instead of exporting dataDependencies', define and export a function

lambdaDependencies :: Dependencies -> Lambda rep -> [Names] -> [Names]

that essentially handles lambdas like you to explicitly in opDependencies. The [Names] argument should be a list of the same length as the number of parameters of the lambda. We should try to centralise as much of the machinery in the DataDependencies module as possible.

@nhey
Copy link
Member Author

nhey commented Aug 9, 2023

That works well. Do you have any clue as to why unused operations (adds) are eliminated for map and reduce tests, but not for the scan test? I'm printing the dependencies at various places and they appear to be similar for map and scan.

The tests are in tests/dependence-analysis/{map0,scan0,reduce0}.fut.

src/Futhark/IR/SOACS/SOAC.hs Outdated Show resolved Hide resolved
@athas
Copy link
Member

athas commented Aug 9, 2023

The operator in the scan test is more complicated than the one for the reduce test. Are you sure it works for reductions?

@athas
Copy link
Member

athas commented Aug 9, 2023

Oh, but the actual reason is that the removal of dead reduction results is by the dedicated rule removeDeadReduction. There is no similar rule for scans. It would have to be added (and would be very similar).

nhey added 8 commits September 1, 2023 23:44
Only issue419.fut hits this case. (Perhaps there will be more
once the IR/GPU IR/SegOp implementations of opDependencies are less
naive---they just use freeIn atm.)
Need to add dedicated test case.
@nhey
Copy link
Member Author

nhey commented Sep 10, 2023

Should JVP and VJP be handled in opDependencies? Otherwise it's ready for review.

@athas
Copy link
Member

athas commented Sep 10, 2023

Yes, they should. I'll review this PR this week.

@nhey nhey marked this pull request as ready for review September 11, 2023 11:14
@@ -27,6 +33,23 @@ dataDependencies' ::
Dependencies
dataDependencies' startdeps = foldl grow startdeps . bodyStms
where
grow deps (Let pat _ (WithAcc inputs lam)) =
let input_deps = map (mconcat . depsOfWithAccInput) inputs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use foldMap.

@nhey
Copy link
Member Author

nhey commented Sep 28, 2023

I cannot reproduce the failing tests for issue456.fut locally, but will have a look at the dependencies being generated manually. But I don't get why they would fail now and not before.

@athas
Copy link
Member

athas commented Sep 28, 2023

I will also take a look. It's possible that your change is not a fault, but simply reveals a bug in some other simplification.

@nhey
Copy link
Member Author

nhey commented Sep 28, 2023

Ok, I'll leave it for now. The dependencies are as I would expect for the standard and multicore pipelines. I couldn't produce any differences between main branch and this branch by adding tests to the program. The IR after the standard pipeline is different, but looks correct to me. (However, I guess the error occurs in one of the simplify passes in the multicure/ispc/wasm pipelines, so that may be irrelevant.)

@athas
Copy link
Member

athas commented Oct 2, 2023

I also cannot reproduce locally. Hypothesis: the error only occurs after you merge master into this branch (which GitHub will do before running CI).

@athas
Copy link
Member

athas commented Oct 2, 2023

I found a fix for the real bug this uncovered, but the reason it happens is that the data dependency analysis is now smart(?) enough that we simplify away the kind of manual double buffering that Cosmin is fond of. I guess he'll find out the hard way!

@athas athas added the run-benchmarks Makes GA run the benchmark suite. label Oct 2, 2023
@athas athas merged commit 46b1e0b into diku-dk:master Oct 2, 2023
35 checks passed
CKuke pushed a commit to CKuke/futhark-seq that referenced this pull request Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-benchmarks Makes GA run the benchmark suite.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants