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

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

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
Copy link
Contributor Author

dpvanbalen commented Nov 25, 2021

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

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 (:

…stencil dimensions, use Sugar.Stencil instead of Smart.hs.Stencil
@dpvanbalen
Copy link
Contributor Author

dpvanbalen commented Dec 9, 2021

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.

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?

@@ -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

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.

@@ -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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants