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

Add blackbox for 'Clash.Sized.Vector.iterateI' #1354

Merged
merged 16 commits into from
Jun 7, 2020
Merged

Conversation

martijnbastiaan
Copy link
Member

@martijnbastiaan martijnbastiaan commented May 28, 2020

Adds blackbox (and associated reduceNonRepPrim/evaluator rule) for Clash.Sized.Vector.iterateI. Works around #1240.


Name Before (ms) After (ms) B/A
FIR 35.73 34.61 1.03
Reducer 771.6 732 1.05
Queens 16.89 16.30 1.03
BundleMapRepeat 21.70 16.88 1.28
PipelinesViaFolds 1083 208.1 5.20
AES 326.6 281.6 1.07
T1354B 508

martijnbastiaan added a commit that referenced this pull request May 28, 2020
martijnbastiaan added a commit that referenced this pull request May 28, 2020
@martijnbastiaan martijnbastiaan marked this pull request as ready for review May 28, 2020 11:58
Copy link
Contributor

@alex-mckenna alex-mckenna left a comment

Choose a reason for hiding this comment

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

A couple of questions about things, but generally looks good. One question that didn't make sense being tagged in a particular area: does this work in Verilog / SystemVerilog? Some parts are explicitly VHDL instead of any backend, but other parts seem to be over every backend.

clash-lib/src/Clash/Normalize/Transformations.hs Outdated Show resolved Hide resolved
clash-lib/src/Clash/Normalize/Transformations.hs Outdated Show resolved Hide resolved
clash-lib/src/Clash/Primitives/DSL.hs Show resolved Hide resolved
clash-lib/src/Clash/Primitives/DSL.hs Show resolved Hide resolved
clash-lib/src/Clash/Primitives/DSL.hs Outdated Show resolved Hide resolved
clash-lib/src/Clash/Primitives/DSL.hs Outdated Show resolved Hide resolved
clash-lib/src/Clash/Primitives/Sized/Vector.hs Outdated Show resolved Hide resolved
@martijnbastiaan
Copy link
Member Author

One question that didn't make sense being tagged in a particular area: does this work in Verilog / SystemVerilog? Some parts are explicitly VHDL instead of any backend, but other parts seem to be over every backend.

Yes this works for all HDLs. The changes regarding the this are just an improvement on the DSL that was donated/upstreamed by Chris over at #1352. I haven't used any of those function for the iterateI blackbox though :)

martijnbastiaan added a commit that referenced this pull request May 28, 2020
martijnbastiaan added a commit that referenced this pull request May 28, 2020
@martijnbastiaan
Copy link
Member Author

Thanks for the review @alex-mckenna! I've implemented your suggestions.

martijnbastiaan added a commit that referenced this pull request May 28, 2020
@alex-mckenna alex-mckenna self-requested a review May 28, 2020 15:43
@martijnbastiaan
Copy link
Member Author

Converting to draft PR. The following example makes Clash get stuck in an infinite loop:

module Test where

import Clash.Prelude

f :: forall n. KnownNat n => Index (n + 1) -> Int -> Int
f n = foldl (.) id lanes
 where
  lanes :: Vec (n + 1) (Int -> Int)
  lanes = map (\i -> if i < n then id else id) (iterateI succ 0) 

topEntity :: Int -> Int
topEntity = f (1 :: Index 5) 

@martijnbastiaan martijnbastiaan marked this pull request as draft May 28, 2020 15:45
@martijnbastiaan martijnbastiaan force-pushed the workaround1240 branch 2 times, most recently from 3a5ce52 to 4076b79 Compare June 2, 2020 09:03
martijnbastiaan added a commit that referenced this pull request Jun 2, 2020
martijnbastiaan added a commit that referenced this pull request Jun 3, 2020
@martijnbastiaan martijnbastiaan force-pushed the workaround1240 branch 2 times, most recently from a73b9d7 to 05bd738 Compare June 4, 2020 08:07
martijnbastiaan added a commit that referenced this pull request Jun 4, 2020
@martijnbastiaan martijnbastiaan marked this pull request as ready for review June 4, 2020 20:42
Prevents huge expressions, therefore improving compilation times
Prevents huge expressions, therefore improving compilation times
Prevents huge expressions, therefore improving compilation times
Prevents huge expressions, therefore improving compilation times
@martijnbastiaan martijnbastiaan force-pushed the workaround1240 branch 3 times, most recently from 7b5f5b5 to 3f96233 Compare June 5, 2020 07:11
@alex-mckenna alex-mckenna self-requested a review June 5, 2020 08:03
clash-lib/src/Clash/Rewrite/Util.hs Outdated Show resolved Hide resolved
clash-lib/src/Clash/Rewrite/Util.hs Outdated Show resolved Hide resolved
@martijnbastiaan
Copy link
Member Author

Testing on a private, bigger codebase revealed a 20% speedup @christiaanb :). I've applied the review suggestions, so I think this is good to go. (If merging, it's probably best to create a merge commit for this one, as there's quite a lot of info in this thread.)

@martijnbastiaan martijnbastiaan merged commit aea0a94 into master Jun 7, 2020
@martijnbastiaan martijnbastiaan deleted the workaround1240 branch June 7, 2020 19:33
@christiaanb
Copy link
Member

Any reason not to backport to 1.2?

@martijnbastiaan
Copy link
Member Author

It's a bit of work, most of which I've done in backport-workaround1240, but it needs updating to include the latest force pushes.

Comment on lines +174 to +175
( Case vec elTy [(pat, Var el)]
, Case vec restTy [(pat, Var rest)] )
Copy link
Member

Choose a reason for hiding this comment

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

@martijnbastiaan Doesn't this duplicate (at least compiler) work for the vec term?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly / probably. How would you write it?

There should also be a "quick" path that just extract the element if it sees a data constructor.

martijnbastiaan added a commit that referenced this pull request Jun 11, 2020
martijnbastiaan added a commit that referenced this pull request Jun 11, 2020
martijnbastiaan added a commit that referenced this pull request Jul 16, 2020
PR #1354 makes code in the wild fail
@martijnbastiaan martijnbastiaan mentioned this pull request Jul 16, 2020
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