-
Notifications
You must be signed in to change notification settings - Fork 118
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
base: master
Are you sure you want to change the base?
Unary Stencil #512
Conversation
Looking again, perhaps it is better to go with the |
This version now uses
We could also use a self-rolled Solo (since GHC.Tuple is wildly different in 8.10), and/or e.g. also export 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 |
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 |
Also there are currently no tests for |
…stencil dimensions, use Sugar.Stencil instead of Smart.hs.Stencil
I changed 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! |
Regarding |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zero as well?
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 1e
s. 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 changeTup
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: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!