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

Port barrier tests from Lift #220

Merged
merged 4 commits into from
Feb 17, 2022
Merged

Port barrier tests from Lift #220

merged 4 commits into from
Feb 17, 2022

Conversation

Bastacyclop
Copy link
Member

This ports more than 40 barrier unit tests from the Lift repository to ours.
Overall, Lift seems to generate less barriers, but also to generate more invalid barriers (see point 2.). The barrier insertion of Shine is more conservative and often looks more correct to me, but could be improved for performance (see point 1.).

  1. In many tests, Lift is able to generate less barriers. For Shine to do the same, the dependency analysis for barrier insertion needs to be more precise, currently it is very coarse-grain.
  2. In many tests, Lift generates invalid barriers (e.g. using the wrong address space flag, or forgetting dependencies between loop round-trips), but the code seems to work by chance.
  3. Some tests in Lift inject work sizes to ensure that some loops are only taken once, I ported them over without injecting the sizes. This should be corrected when fixing Unnecessary end-of-loop barriers #212
  4. Some generated barriers might not be reached by all threads (already mentioned in Work on barrier elimination #80). This needs to be solved at some point, Lift had a detection system to throw errors in such cases, but I think we could do better by automatically transforming the invalid code into valid code.
  5. Some tests from Lift create writing races with patterns such as mapSeq/reduceSeq which are outside of a mapLocal. I think such programs should be rejected and considered ill formed.
  6. In many tests, Lift generates less barriers by using more memory allocation, while Rise generates more barriers at the benefit of using less memory allocation. This is a hard-wired trade-off in both frameworks.
  7. Some tests have invalid access annotations when ported to Rise, this is an orthogonal problem but deserves more thought.
  8. Some tests generate record of arrays which breaks our code generation, another orthogonal problem.

@johanneslenfers
Copy link
Member

Porting the barrier tests from Lift makes sense to me.

Just for my understanding: Is point 2 captured by the tests? So in Lift the tests would pass because the code works? Do these tests pass for Rise? Would it make sense to check for invalid barriers rather than working code?

Are you going to keep track of these points as issue(s)?

@Bastacyclop
Copy link
Member Author

Do these tests pass for Rise? Would it make sense to check for invalid barriers rather than working code?

The tests in Rise are not actually running the generated code (maybe they should), instead they count barriers as in:

    "barrier".r.findAllIn(code).length shouldBe 2
    """barrier\(CLK_LOCAL_MEM_FENCE\)""".r.findAllIn(code).length shouldBe 2

It's not bullet proof as the right amount of barriers may appear in the wrong place, so the tests could be more sophisticated.

Are you going to keep track of these points as issue(s)?

  1. Precision of dependency analysis for barrier insertion #225
  2. Lift issue
  3. Unnecessary end-of-loop barriers #212
  4. Ensuring that barriers are encountered by all work-items #224
  5. Low-level Rise sanity check #226
  6. not sure if that's really an issue
  7. Some Lift programs do not have valid access annotations in Rise #228
  8. Records of variable length arrays do not generate valid code #227

@Bastacyclop Bastacyclop merged commit cfa3a1b into main Feb 17, 2022
@Bastacyclop Bastacyclop deleted the barrier-tests-from-lift branch February 17, 2022 16:59
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