Repository navigation
Make sure that 'length' can be inlined (Fixes #97) - #155
Merged
Merged
Conversation
Previously there was a cycle in the dependency graph of functions,
consisting of {stream, clone, length, unsafeCopy}. This was causing GHC
to mark one of these functions, length, as a loop breaker.
This commit breaks this down this strongly-connected component by
removing the edge from length to stream.
Contributor
|
@takano-akio cool! this raises an interesting question: how should we track making sure all these little changes stay the correct ones? @takano-akio @dolio @hvr should we figure out having some sort of benchmark suite integrated into the project? (i feel like we'd need to use something like cabal.project / cabal-new build style tricks to be able to use criterion or friends correctly for this). But we do want to track which of equivalent implementations of different pieces perform better in a given version of vector and ghc. (cause measuring is the only way!) |
Contributor
|
I've done a little experimenting (in my local copy of vector-algorithms), and you can definitely use cabal.project to do a criterion test suite even though it depends on vector. |
Merged
gksato
added a commit
to gksato/vector
that referenced
this pull request
Jun 20, 2020
This change makes fusible 'Data.Vector.Generic.length', which currently can be inlined but doesn't fuse well. Before this revision, 'length' uses non-fusible 'stream''. This revision replaces 'stream'' with fusible 'stream', and at the same time substitute the mutually-recursive occurrences of 'length' with 'basicLength'. Rationale: The current definition was introduced in the commit a811a86. Prior to that commit, 'length' could not even be inlined with older GHCs, which elected 'length' to be a loop-breaker due to the cyclic references among 'clone', 'unsafeCopy', 'length' and 'stream': * 'clone' refers to 'length' and 'unsafeCopy' in its definition; * 'unsafeCopy' refers to 'length' in its definition; * 'length' referred to 'stream' in its definition; * 'stream' refers to none of the above in its definition, but the rule 'New.unstream/stream' rewrites @'New.unstream' ('stream' v)@ with @'clone' v@. This failure of inlining was reported as haskell#97. To be precise, this cycle of references is immaterial with newer GHCs like GHC 8.8.3. Since 'New.unstream' does not occur in the definitions of 'clone', 'unsafeCopy', and 'length', indefinite inlining will not happen anyway. GHC 8.8.3 seems to detect this immateriality of the referencial loop in question and refrains from setting 'length' as a loop-breaker, even with acdcff3 (the parent commit of a811a86). However with older GHCs, including GHC 8.0.2, regarded the cyclic references fatal and marked 'length' as a loop-breaker. Note that GHC 8.0.2 was current at the time a811a86 was introduced. The commit a811a86 resolved this by replacing 'stream' with 'stream'' in the definition of 'length'. The function 'stream' refers to 'clone' in its fusion rule, but 'stream'' doesn't possess any rule. This cut open the cycle of references and made 'length' inlinable. However, since 'stream'' doesn't possess any rule, defining 'length' = 'Bundle.length' . 'stream'' is all the same as setting 'length' = 'basicLength'. This prevents any stream fusion from happening. This commit resolves this problem by resetting 'length' back to be @'Bundle.length' . 'stream'@, and instead replacing cyclic occurrence of 'length' with 'basicLength' (Just *inlined the simplification result of the definition*). Now 'length' is both inlinable and fusible, even with older GHCs. Fixes: haskell#306 See also: haskell#97 haskell#111 haskell#155
Shimuuar
pushed a commit
that referenced
this pull request
Jun 21, 2020
This change makes fusible 'Data.Vector.Generic.length', which currently can be inlined but doesn't fuse well. Before this revision, 'length' uses non-fusible 'stream''. This revision replaces 'stream'' with fusible 'stream', and at the same time substitute the mutually-recursive occurrences of 'length' with 'basicLength'. Rationale: The current definition was introduced in the commit a811a86. Prior to that commit, 'length' could not even be inlined with older GHCs, which elected 'length' to be a loop-breaker due to the cyclic references among 'clone', 'unsafeCopy', 'length' and 'stream': * 'clone' refers to 'length' and 'unsafeCopy' in its definition; * 'unsafeCopy' refers to 'length' in its definition; * 'length' referred to 'stream' in its definition; * 'stream' refers to none of the above in its definition, but the rule 'New.unstream/stream' rewrites @'New.unstream' ('stream' v)@ with @'clone' v@. This failure of inlining was reported as #97. To be precise, this cycle of references is immaterial with newer GHCs like GHC 8.8.3. Since 'New.unstream' does not occur in the definitions of 'clone', 'unsafeCopy', and 'length', indefinite inlining will not happen anyway. GHC 8.8.3 seems to detect this immateriality of the referencial loop in question and refrains from setting 'length' as a loop-breaker, even with acdcff3 (the parent commit of a811a86). However with older GHCs, including GHC 8.0.2, regarded the cyclic references fatal and marked 'length' as a loop-breaker. Note that GHC 8.0.2 was current at the time a811a86 was introduced. The commit a811a86 resolved this by replacing 'stream' with 'stream'' in the definition of 'length'. The function 'stream' refers to 'clone' in its fusion rule, but 'stream'' doesn't possess any rule. This cut open the cycle of references and made 'length' inlinable. However, since 'stream'' doesn't possess any rule, defining 'length' = 'Bundle.length' . 'stream'' is all the same as setting 'length' = 'basicLength'. This prevents any stream fusion from happening. This commit resolves this problem by resetting 'length' back to be @'Bundle.length' . 'stream'@, and instead replacing cyclic occurrence of 'length' with 'basicLength' (Just *inlined the simplification result of the definition*). Now 'length' is both inlinable and fusible, even with older GHCs. Fixes: #306 See also: #97 #111 #155
lehins
pushed a commit
that referenced
this pull request
Jan 16, 2021
This change makes fusible 'Data.Vector.Generic.length', which currently can be inlined but doesn't fuse well. Before this revision, 'length' uses non-fusible 'stream''. This revision replaces 'stream'' with fusible 'stream', and at the same time substitute the mutually-recursive occurrences of 'length' with 'basicLength'. Rationale: The current definition was introduced in the commit a811a86. Prior to that commit, 'length' could not even be inlined with older GHCs, which elected 'length' to be a loop-breaker due to the cyclic references among 'clone', 'unsafeCopy', 'length' and 'stream': * 'clone' refers to 'length' and 'unsafeCopy' in its definition; * 'unsafeCopy' refers to 'length' in its definition; * 'length' referred to 'stream' in its definition; * 'stream' refers to none of the above in its definition, but the rule 'New.unstream/stream' rewrites @'New.unstream' ('stream' v)@ with @'clone' v@. This failure of inlining was reported as #97. To be precise, this cycle of references is immaterial with newer GHCs like GHC 8.8.3. Since 'New.unstream' does not occur in the definitions of 'clone', 'unsafeCopy', and 'length', indefinite inlining will not happen anyway. GHC 8.8.3 seems to detect this immateriality of the referencial loop in question and refrains from setting 'length' as a loop-breaker, even with acdcff3 (the parent commit of a811a86). However with older GHCs, including GHC 8.0.2, regarded the cyclic references fatal and marked 'length' as a loop-breaker. Note that GHC 8.0.2 was current at the time a811a86 was introduced. The commit a811a86 resolved this by replacing 'stream' with 'stream'' in the definition of 'length'. The function 'stream' refers to 'clone' in its fusion rule, but 'stream'' doesn't possess any rule. This cut open the cycle of references and made 'length' inlinable. However, since 'stream'' doesn't possess any rule, defining 'length' = 'Bundle.length' . 'stream'' is all the same as setting 'length' = 'basicLength'. This prevents any stream fusion from happening. This commit resolves this problem by resetting 'length' back to be @'Bundle.length' . 'stream'@, and instead replacing cyclic occurrence of 'length' with 'basicLength' (Just *inlined the simplification result of the definition*). Now 'length' is both inlinable and fusible, even with older GHCs. Fixes: #306 See also: #97 #111 #155
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously there was a cycle in the dependency graph of functions,
consisting of {stream, clone, length, unsafeCopy}. This was causing GHC
to mark one of these functions, length, as a loop breaker.
This commit breaks this down this strongly-connected component by
removing the edge from length to stream.
I have confirmed that the test case in #97 is compiled into nice code with this change.