accelerate icon indicating copy to clipboard operation
accelerate copied to clipboard

Unary Stencil

Open dpvanbalen opened this issue 4 years ago • 6 comments

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)
  • [x] 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 avatar Nov 25 '21 18:11 dpvanbalen

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 avatar Nov 25 '21 18:11 dpvanbalen

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

dpvanbalen avatar Nov 25 '21 21:11 dpvanbalen

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 avatar Dec 09 '21 11:12 tmcdonell

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

tmcdonell avatar Dec 09 '21 11:12 tmcdonell

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

dpvanbalen avatar Dec 09 '21 16:12 dpvanbalen

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 avatar Apr 09 '24 09:04 tomsmeding