Skip to content

Conversation

DilumAluthge
Copy link
Member

No description provided.

@DilumAluthge
Copy link
Member Author

If this works well, we can reproduce this pattern in other repos where the test suite performance really slows down when code coverage is enabled.

@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #8 (4e85e7e) into master (cf3b37a) will decrease coverage by 5.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
- Coverage   82.92%   77.87%   -5.06%     
==========================================
  Files           7        7              
  Lines         123      113      -10     
==========================================
- Hits          102       88      -14     
- Misses         21       25       +4     
Impacted Files Coverage Δ
src/Octavian.jl 100.00% <ø> (ø)
src/macrokernel.jl 54.54% <ø> (-37.77%) ⬇️
src/matmul.jl 100.00% <ø> (ø)
src/utils.jl 71.42% <ø> (-1.91%) ⬇️
src/block_sizes.jl 93.75% <100.00%> (-0.37%) ⬇️
src/types.jl 44.44% <100.00%> (ø)
src/macros.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf3b37a...4e85e7e. Read the comment docs.

@DilumAluthge
Copy link
Member Author

Wow, look how fast!

@DilumAluthge DilumAluthge marked this pull request as ready for review December 29, 2020 18:03
@DilumAluthge DilumAluthge marked this pull request as draft December 29, 2020 18:21
@DilumAluthge DilumAluthge marked this pull request as ready for review December 29, 2020 18:23
@chriselrod
Copy link
Collaborator

chriselrod commented Dec 29, 2020

Note that different matrix sizes hit different code paths.
But I think we're actually okay here, because being transposed also changes the codepaths, meaning it should be hitting them all anyway.

@chriselrod
Copy link
Collaborator

Inside matmul! we have:

if M * K  _Mc * _Kc && A isa DenseArray && C isa StridedArray && B isa StridedArray && #
        (stride(A,2)  72 || (iszero(stride(A,2) & (VectorizationBase.pick_vector_width(eltype(A))-1)) && iszero(reinterpret(Int,pointer(A)) & 63)))
    macrokernel!(C, A, B, _α, _β)
    return C
end

a check for whether we should just call macrokernel! directly.
And then similarly:

do_not_pack_B = (B isa DenseArray && (K * N  _Kc * _Nc)) || N  LoopVectorization.nᵣ

To check if we should skip packing B.
Because of the DenseArray checks, we'll still take both sides of each.

@DilumAluthge DilumAluthge merged commit 72b511a into master Dec 29, 2020
@DilumAluthge DilumAluthge deleted the dpa/coverage branch December 29, 2020 18:59
@DilumAluthge
Copy link
Member Author

I wonder if this kind of pattern could be useful in other test suites (LoopVectorization.jl, VectorizationBase.jl, PaddedMatrices.jl, Gaius.jl, etc.).

Of course, it's only useful in cases where the test suite time greatly increases between coverage disabled and coverage enabled.

@chriselrod
Copy link
Collaborator

The beta != 0 code path actually isn't getting hit anymore.
It gets set to 1 for the second+ iteration of the K loop, but we could also just call matmul!(C, A, B, true, true), for example.

@DilumAluthge
Copy link
Member Author

The beta != 0 code path actually isn't getting hit anymore.
It gets set to 1 for the second+ iteration of the K loop, but we could also just call matmul!(C, A, B, true, true), for example.

Yeah worst case we can just add unit tests to make up the coverage. I'll open a PR in a few minutes.

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.

2 participants