rise-lang / shine

The Shine compiler for the RISE language
https://rise-lang.org
MIT License
73 stars 8 forks source link

Port barrier tests from Lift #220

Closed Bastacyclop closed 2 years ago

Bastacyclop commented 2 years ago

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 #212
  4. Some generated barriers might not be reached by all threads (already mentioned in #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 commented 2 years ago

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 commented 2 years ago

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. 225

  2. Lift issue
  3. 212

  4. 224

  5. 226

  6. not sure if that's really an issue
  7. 228

  8. 227