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

Declutter Internal Shape of Vector #134

Merged
merged 23 commits into from
Apr 23, 2015
Merged

Declutter Internal Shape of Vector #134

merged 23 commits into from
Apr 23, 2015

Conversation

emwap
Copy link
Member

@emwap emwap commented Jan 5, 2015

As a step on the way to implementing #91, this pull request declutters the internal representation of Pull.
Pull DIM0 a is represented as a scalar Internal a.
Pull DIM1 a is represented as a list [Internal a].

With this change there should be no need for the Feldspar.SimpleVector module.

However, it breaks some of the assumptions in the ExternalProgram module in feldspar-compiler.

/cc @emilaxelsson, @josefs, @pjonsson

This commit changes the internal representation of `Vector` to remove
tuples on 0- and 1-dimensional vectors. They are now represented as
scalars and lists respectively.
@pjonsson
Copy link
Member

pjonsson commented Jan 5, 2015

Doesn't the other vector types (Push, Manifest, etc) need the same kind of patch?

The new type family should morally be closed. What advantage does the closed type family bring if we have to have the open type family around anyways?

ExternalProgram is meant as a tool for debugging and testing the compiler so assume that there is an expert user for that. I assume we need some special hardwired knowledge in there to recognise DIM0/DIM1 arrays.

@emwap
Copy link
Member Author

emwap commented Jan 5, 2015

Yes, I will add instances for Push and Manifest.

The only "advantage" is compatibility with older GHC (pre 7.8) versions. With 7.10 coming out soon, I guess we could remove the open family.

Would you be ok with putting ExternalProgram behind a cabal-flag (default false) and temporarily breaking it?

@pjonsson
Copy link
Member

pjonsson commented Jan 5, 2015

My question was actually the other way around: if open type families are enough to get the functionality we want, why add extra code for closed type families? Haskell support on RHEL seems to have stopped updating around 7.6 so I'm a bit split about deprecating that right now.

Add the cabal flag for now and I'll patch ExternalProgram at a later date. Since this is still hot in your cache: can you please summarise the data structure layout changes from this patch so I don't have to reverse-engineer that when I get to updating ExternalProgram.

@emwap
Copy link
Member Author

emwap commented Jan 5, 2015

The motivation for the closed family is as you said before, it's morally closed and I wanted to express that to GHC. I guess we could consider any drawbacks of leaving it open.

Pull DIM0 a is represented as Internal a.
Pull DIM1 a is represented as [Internal a].
Pull DIM2 a is represented as ([Length], [Internal a]).

Only the DIM0 and DIM1 cases have changed and the representation for DIM2 and above is the same as before.

@pjonsson
Copy link
Member

pjonsson commented Jan 7, 2015

ExternalProgram might actually work as is with that representation.

@emwap
Copy link
Member Author

emwap commented Jan 8, 2015

It does, see branch Feldspar/feldspar-compiler#197

@pjonsson
Copy link
Member

Ok, so can this be merged now?

@emwap
Copy link
Member Author

emwap commented Jan 12, 2015

Yes, but that will break test-cases in Feldspar/feldspar-compiler. I still have some quirks to sort out there.

@josefs
Copy link
Contributor

josefs commented Jan 12, 2015

A suggestion wrt to Manifest: the easiest way to deal with Manifest is probably to define it as follows:

data Manifest sh a = Syntax a => Manifest (InternalShape sh a)

emwap added a commit that referenced this pull request Apr 23, 2015
Declutter Internal Shape of Vector
@emwap emwap merged commit a81e9e4 into master Apr 23, 2015
@emwap emwap deleted the internal_shape branch April 24, 2015 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants