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

Support embedded pattern matching #460

Merged
merged 43 commits into from Jul 16, 2020

Conversation

tmcdonell
Copy link
Member

Description

This patch primarily adds support for sum data types in the Accelerate expression language.

The backends still need to be updated before this can be merged.

Motivation and context

Fixes: #87
Fixes: #100
Fixes: #249
Fixes: #263

How has this been tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

* Splits the modules D.A.A.Array.{Sugar,Representation} into several  modules under the tree D.A.A.{Sugar,Representation}.*

* Remove D.A.A.Trafo.Base, which was a lazy dumping ground for unrelated functionality

* Move various definitions around into new modules, for example putting lift and rnf instances close to where those data structures are defined, rather than dumping them all into D.A.A.AST
# Conflicts:
#	src/Data/Array/Accelerate/AST.hs
#	src/Data/Array/Accelerate/Language.hs
#	src/Data/Array/Accelerate/Pretty/Graphviz.hs
#	src/Data/Array/Accelerate/Pretty/Print.hs
#	src/Data/Array/Accelerate/Smart.hs
#	src/Data/Array/Accelerate/Trafo/Fusion.hs
#	src/Data/Array/Accelerate/Trafo/LetSplit.hs
#	src/Data/Array/Accelerate/Trafo/Sharing.hs
* add tagsR field to Elt, to be used for pattern matching
* Bool is no longer a primitive type
Don’t let cabal compile .c files, which perpetually leads to linking problems. Instead use TH to tell GHC about these files directly.
Technically this restriction is based on template-haskell, which is required to work around limitations of Cabal
also update TH generator to define patterns for simple product types in terms of Pattern
@tmcdonell tmcdonell marked this pull request as draft July 1, 2020 16:27
@@ -89,16 +83,14 @@ isRight x = tag x == 1
-- instead.
--
fromLeft :: (Elt a, Elt b) => Exp (Either a b) -> Exp a
fromLeft x = a
where T3 _ a _ = asTuple x
fromLeft (Exp e) = Exp $ SmartExp $ Prj PairIdxRight $ SmartExp $ Prj PairIdxLeft $ SmartExp $ Prj PairIdxRight e
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the goal of this PR was to allow users to define custom sum types. I hope they don't have to write this? Or is this line just avoiding generics, to be more efficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

nah, this is just to define the unsafe fromLeft and fromRight functions. These existed from before this patch, I'm just updating them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess what I mean is, if defining 'your own' Either datatype (including all these 'isRight' and 'fromLeft' functions) is now possible on the user side, it would make sense to show off the `intended' way here instead of using Accelerate internals? Now that this vesion already works, that might be wasted effort, but I was surprised to see this after the paper promised a very neat interface

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I understand what you mean now. I updated the functions in that module and added an example to the 'match' docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

isRight (Exp e) = Exp $ SmartExp $ (SmartExp $ Prj PairIdxLeft e) Pair SmartExp Nil
I assume this is the same as something like:
isRight = match \case
Right_ _ -> True
Left_ _ -> False

This looks great! I was already planning on rewriting the sudoku solver, now that I know more about Accelerate (even changing a 'run' to 'runN' should already be an improvement I think, it now repeatedly compiles the same Acc program), and I might see what happens to the readability and performance if I use some nice ADTs instead of bitwise hacking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. I'm just cheating because I know the tag bits for Right and True happen to be the same. I'll add a comment mentioning this.

At some point I went and added pattern synonyms to the sudokus code. Apparently I left it in a half-committed state, but I put it here: https://github.com/tmcdonell/Sudokus

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that already looks much better! I'll put some more effort into that once I'm done :)

@tmcdonell tmcdonell marked this pull request as ready for review July 6, 2020 11:05
@ivogabe
Copy link
Contributor

ivogabe commented Jul 15, 2020

Looks good, I tested it on my delta stepping implementation and that worked fine (after changing the calls to permute of course). I have some minor suggestions / questions:

  • Do we want to make match required for all uses of pattern matching (like it is currently), or do we want to automatically handle that without match on top-level functions (eg functions directly passed to a combinator)? That would allow you to write map (\case ...). I thought that was the initial idea, that should catch most of the uses of pattern matching. However it might be better to make it required everywhere as that may give better understanding, so I'm not sure about this yet.
  • Can we do anything for pattern matching on integers? We should at least update the error message that you would currently get to also say something about pattern matching: Prelude.Eq.(==) applied to EDSL types: use Data.Array.Accelerate.(==) instead
  • You renamed TupleType to TypeR, I agree that TupleType was a bad name but would EltR be an even better name? We could then also go for symmetry with ArraysR / ArraysRepr and EltR / EltRepr.
  • Can you elaborate on the changes for characters and booleans?
  • Should we expose the patterns from the main module? That seems useful especially for Just_ and Nothing_, as you will now need them to do permutations.

@tmcdonell
Copy link
Member Author

  • Sadly there is already a type called EltR in the refactor. TypeR is at least better than TupleType, so I'm happy with it for now.
  • Char in Haskell is a (somewhat magic) newtype around Word32, so I just did the same in Accelerate. This is similar to how we previously had newtypes like CInt as primitives, and have since removed those.
  • Elt Bool can be automatically derived now, with EltR Bool = (Word8, ()). I was initially very happy to be able to remove this, but after having to make all the changes I'm not sure it was a good decision overall... I'm tempted to put it back.
  • Regarding literal patterns, a function such as:
isZero :: Num a => Exp a -> Exp Bool
isZero 0 = True_
isZero _ = False_

will be desugared into:

isZero x =
  case x == fromInteger 0 of
    True  -> True_
    False -> False_

We can't use the built in (==) because that won't work on embedded terms, and if we use RebindableSyntax to use our own (==) :: Eq a => Exp a -> Exp a -> Exp Bool then the case statement will be type incorrect. I'm not sure if there is a way around this \:

@ivogabe
Copy link
Contributor

ivogabe commented Jul 15, 2020

Thanks, that clears things up for me :).

Ah, I thought that the type EltR was called EltRepr, but that's not the case. So we could then rename EltR to EltRepr, that may make consistent with ArraysR and ArrRepr. (But that's again a refactor through the whole program, so maybe we should just be happy with the current situation)

@tmcdonell tmcdonell merged commit e93a0d2 into AccelerateHS:master Jul 16, 2020
@tmcdonell tmcdonell deleted the wip/sum-types branch June 27, 2021 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants