mikeizbicki / subhask

Type safe interface for working in subcategories of Hask
BSD 3-Clause "New" or "Revised" License
418 stars 43 forks source link

Initial implementations of Accelerate backends for Vector and Matrix #39

Closed o1lo01ol1o closed 8 years ago

o1lo01ol1o commented 8 years ago

I've made a first pass through the implementations by copying all or most of the instances for SVector and the newly added Matrix types. There's some problems when the instance methods return Accelerate types (eg, A.Exp) instead of whatever is expected, but I'm not sure how to amend this. There are also a couple of points where some types can't be deduced, maybe you can advise on what's missing there? I've commented out the sections that need to be resolved. When they're working I can finish the tests in the examples folder.

I used the current master branches of the acclerate packages as the hackage packages are now quite old. The LLVM package wouldn't build when I added it as a dependency, some problem with a missing cabal file, I've opened an issue, maybe it can be fixed. Repa is 5 years without a commit so I left it out in favor of LLVM (which out performed it even 5 years ago).

Let me know how to handle the various issues / what needs to be changed and maybe we can get this finished this week.

mikeizbicki commented 8 years ago

Awesome work! This is super cool :)

I've just briefly glanced through your changes at this point. It's a little bit hard to review them right now though because the changes are spread over multiple commits and intermixed with a bunch of other code. Can you:

  1. move all of the Accelerate specific code into a new Accelerate folder under the Algebra folder; it'd be good to create separate Vector.hs and Matrix.hs and whatever other files in there
  2. squash the commits

for me?

o1lo01ol1o commented 8 years ago

CUDA is working; I'm having problems building the acclerate-llvm packages currently so I haven't been able to test those. I'll move everything to an Accelerate directory and see if I can't get the instance methods working and then get back to you.

mikeizbicki commented 8 years ago

Oops! I'm really sorry I didn't see this earlier :( I'm super stoked about this though! It looks awesome!

I've merged your code into a separate branch o1lo01ol1o-tpierson_gpu_vec for now. There were a few minor edits needed to make it compatible with master, so try to work from the modified code. The last step to get this merged into master is adding to the tests suite. See, for example, how the other vectors are tested: https://github.com/mikeizbicki/subhask/blob/master/test/TestSuite.hs#L43 and just replicate that. I don't think you should have to modify any other files.

o1lo01ol1o commented 8 years ago

Hey, no worries, I was just about to write a note: getting everything working is proving trickier than I thought, there's lots head scratching about getting GHC to infer the correct types under the accelerate types in a given context. (I try one thing, do that all the way through, fail, and then try another). I'm going to get as much as I can working, squash them, and then maybe you can glance over it before I move onto test. (There's a couple instances that throw errors I still don't understand ...).

I think probably it's going to take using it a bit in some code to work through some of the missing instances / make sure the computation graphs actually work in non-trivial cases. There's also some other Acclerate api stuff to get in, like streams and flow control with kernel execution, but comes later.

o1lo01ol1o commented 8 years ago

I made a couple inline comments the current major stumbling blocks, if I can get those resolved, I'm hoping there should only be a few more in those instances and getting the dagger type working. Could you take a glance and see if you have any thoughts?

I still have some apprehension about nested parallelism and kernel management with the current design. (also we'll have to figure out how to make your test suit call the correct execute functions for these types).

mikeizbicki commented 8 years ago

About FiniteModule and Int: No, there's nothing inherent about the index needing to be an Int, it's just traditional. It'd be theoretically just fine to remove that constraint. This might make type inference a bit more of a pain in some circumstances, but I think that's a fine tradeoff for now. The same is true for the return type of dim. So my advice here is to modify the class constraints however is necessary to get everything to type check.

About the Bool problem: This probably stems from a Logic a ~ Bool constraint somewhere (or equivalently a ClassicalLogic a constraint). There's two ways these constraints come up. First, when using types defined in the standard Prelude, they use boolean logics. Second, GHC 7.10's type checker isn't powerful enough to handle a proper equality hierarchy, and I occassionally added these constraints as a workaround. There's currently a branch for GHC 8.0 that has a better story with the numeric hierarchy (specifically because of the UndecidableSuperClasses extension). Upgrading to that branch might fix the problem.

I just looked at the definition of ValidACCVector and found an Ord constraint. In the master branch, it is defined as

type Ord a = (Ord_ a, ClassicalLogic a)

So the constraint you really want is Ord_. This is one of the warts I mentioned about the class hierarchies in master.

o1lo01ol1o commented 8 years ago

Just an update, I started to move to ghc 8.0 as I was unable to resolve the Bool constraint problem but have run into some dependency snags with accelerate. I've also become swamped with work so will only be making incremental progress until things lighten up. I'll post back when i have some updates.

mikeizbicki commented 8 years ago

Okay, thanks for the update.

On Sat, Jul 2, 2016 at 9:27 AM, o1lo01ol1o notifications@github.com wrote:

Just an update, I started to move to ghc 8.0 as I was unable to resolve the Bool constraint problem but have run into some dependency snags with accelerate. I've also become swamped with work so will only be making incremental progress until things lighten up. I'll post back when i have some updates.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mikeizbicki/subhask/pull/39#issuecomment-230110110, or mute the thread https://github.com/notifications/unsubscribe/ABAP1pzEN2sm1-6W2pfjIU3KKVm02xNCks5qRpGPgaJpZM4IxnJS .

o1lo01ol1o commented 8 years ago

closing this in favor of the ghc 8.0 branch PR. https://github.com/mikeizbicki/subhask/pull/54