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.

Sign up for GitHub

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

Jump to bottom

Unary Stencil #512

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Unary Stencil #512

wants to merge 3 commits into from

Conversation

dpvanbalen
Copy link
Contributor

@dpvanbalen dpvanbalen commented Nov 25, 2021 •
edited
Loading

Description
Adds the option to have a unary dimension in a stencil.

Motivation and context
Cleans up the interface a bit, and using such a unary dimension should also make the generated code faster.
There is one open question: Currently, I just use e as the unary stencil of 1 es. Alternatively, we could use something like https://hackage.haskell.org/package/base-4.16.0.0/docs/Data-Tuple.html#t:Solo to represent the unary tuple, and change Tup accordingly.
Downside of this would be more clutter, but the upside is that it is more clear which dimension you're stencilling over: As the example in the haddoc shows, a 1x5 and 5x1 stencil now look exactly the same.

How has this been tested?
Tests pass

Types of changes
What types of changes does your code introduce? Put an x in all the boxes that apply:

  • 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
Go over all the following points, and put an x in all the boxes that apply. If you're unsure about any of these, don't hesitate to ask. We're here to help!

  • 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

@dpvanbalen dpvanbalen mentioned this pull request Nov 25, 2021
Open
8 tasks
@dpvanbalen
Copy link
Contributor Author

dpvanbalen commented Nov 25, 2021 •
edited
Loading

Looking again, perhaps it is better to go with the Solo route: The example in the haddoc is not run by the doctest, and trying myself I get a different answer. I assume that it interprets both stencils to be horizontal -- is there some overlapping instances going on? How does it make this choice? It probably needs a Solo-like constructor to be able to make the right choice :))
Also, I just noticed that Data.Array.Accelerate.Sugar.Stencil also exists. I didn't add my instances there, and nothing complains.. is it relevant?

@dpvanbalen
Copy link
Contributor Author

dpvanbalen commented Nov 25, 2021 •
edited
Loading

This version now uses Solo, and now this example works as expected:

  let
    convolve5x1 :: [A.Exp Double] -> A.Stencil5x1 Double -> A.Exp Double
    convolve5x1 kernel (A.Solo (a,b,c,d,e))
      = Prelude.sum $ Prelude.zipWith (*) kernel [a,b,c,d,e]
   
    convolve1x5 :: [A.Exp Double] -> A.Stencil1x5 Double -> A.Exp Double
    convolve1x5 kernel (A.Solo a, A.Solo b, A.Solo c, A.Solo d, A.Solo e)
      = Prelude.sum $ Prelude.zipWith (*) kernel [a,b,c,d,e]
    gaussian :: [A.Exp Double]
    gaussian = [0.06136,0.24477,0.38774,0.24477,0.06136]

  in   A.stencil (convolve5x1 gaussian) A.clamp
     . A.stencil (convolve1x5 gaussian) A.clamp

We could also use a self-rolled Solo (since GHC.Tuple is wildly different in 8.10), and/or e.g. also export pattern T0 :: Exp () pattern T1 :: Exp a -> Exp (Solo a)

I still haven't touched the Sugar.Stencil file, and everything works -- it does not even seem to be imported anywhere, nor is it in the .cabal file

@tmcdonell
Copy link
Member

Good question r.e. Sugar.Stencil... I'm guessing that that code used to live in Smart and... checks wait, that code still exists in Smart. Uh, I think that was not properly cleaned up in the conversion to representation types?

Needing to use Solo is unfortunate, is it better than what we have now? I not sure but I guess so?

@tmcdonell
Copy link
Member

Also there are currently no tests for Nx1 or 1xN stencils, so we should add those (:

@dpvanbalen
Self-roll Unary (because GHC.Tuple is unstable), add tests for unary …
a3f2bc2 
…stencil dimensions, use Sugar.Stencil instead of Smart.hs.Stencil
@dpvanbalen
Copy link
Contributor Author

dpvanbalen commented Dec 9, 2021 •
edited
Loading

I changed Solo to newtype Unary a = a, because GHC.Tuple doesn't export Solo for GHC<9.
Also added some tests, and moved the stencil logic from Smart to Sugar.Stencil.

I agree that it would be optimal if we didn't need Unary or Solo, but it seems like the only way to make the types work out. It is probably still a more sensible idiom than (_, a, _), especially because this also has performance benefits!
I wasn't sure where to define it though, it is now in Pattern

@tomsmeding
Copy link
Member

Regarding Solo not being available on older GHCs: there is a compatibility package OneTuple on Hackage that re-exports GHC's own Solo for new enough GHCs. I think that would give the best compatibility story.

tomsmeding
tomsmeding reviewed May 3, 2024
View reviewed changes
Copy link
Member

@tomsmeding tomsmeding left a comment

Choose a reason for hiding this comment

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

Some minor things.
Does this need a PR on accelerate-llvm too?

src/Data/Array/Accelerate/Language.hs
@@ -97,7 +97,7 @@ module Data.Array.Accelerate.Language (
-- * Conversions
ord, chr, boolToInt, bitcast,

) where
Stencil1,Stencil1x3,Stencil1x5,Stencil1x1,Stencil3x1,Stencil5x1) where
Copy link
Member

Choose a reason for hiding this comment

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

A newline before the ) and an indent before the names would be nice here

src/Data/Array/Accelerate/Language.hs
import Prelude ( ($), (.), Maybe(..), Char )
import Data.Array.Accelerate.Sugar.Stencil
Copy link
Member

Choose a reason for hiding this comment

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

The imports were structured before -- whether that's useful is debatable. But I think import Prelude should be separate because it should catch the eye.

src/Data/Array/Accelerate/Representation/Type.hs
@@ -123,5 +123,5 @@ runQ $
in
tySynD (mkName ("Tup" ++ show n)) (map plainTV xs) rhs
in
mapM mkT [2..16]
mapM mkT [0..16]
Copy link
Member

Choose a reason for hiding this comment

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

Zero as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomsmeding tomsmeding tomsmeding left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dpvanbalen @tmcdonell @tomsmeding