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 cblas level 1 routines and corresponding regression tests #364

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tansongchen
Copy link
Collaborator

I'm currently looking on LIT's documentation to learn how to write more comprehensive test, but let's just first put it here.

@tansongchen
Copy link
Collaborator Author

The test passed for higher versions of LLVM but not for lower versions, and I think this is because I generated this .ll file with LLVM 13. @wsmoses Could you maybe tell a little bit on how to generate .ll files that are compatible with lower versions?

enzyme/Enzyme/AdjointGenerator.h Outdated Show resolved Hide resolved
enzyme/test/Enzyme/ReverseMode/blas/cblas_dscal_nomod.ll Outdated Show resolved Hide resolved
@wsmoses
Copy link
Member

wsmoses commented Oct 25, 2021

The test passed for higher versions of LLVM but not for lower versions, and I think this is because I generated this .ll file with LLVM 13. @wsmoses Could you maybe tell a little bit on how to generate .ll files that are compatible with lower versions?

The error is that in lower versions of llvm arguments need actual names, not just %0. Add names to the functions and it should pass?

@tansongchen
Copy link
Collaborator Author

The test passed for higher versions of LLVM but not for lower versions, and I think this is because I generated this .ll file with LLVM 13. @wsmoses Could you maybe tell a little bit on how to generate .ll files that are compatible with lower versions?

The error is that in lower versions of llvm arguments need actual names, not just %0. Add names to the functions and it should pass?

Thanks for the hints. By the way, I am curious about what the life cycle for llvm versions is

@tansongchen
Copy link
Collaborator Author

I uploaded the implementation of level 1 routines and level 2 gemv, but the test system is not working on my computer, I will fix it and add test cases for gemv later

@tansongchen
Copy link
Collaborator Author

Also I noticed that there are some related implementation of gemm at #308 , but both of us are doing this in a bit awkward way. After testing gemv I will try to extract the common portion of many blas routines and do a refactoring

@tansongchen tansongchen changed the title Add cblas sscal/dscal and corresponding regression tests Add cblas level 1 and 2 routines and corresponding regression tests Jan 26, 2022
@vchuravy vchuravy removed their assignment Jan 26, 2022
@tansongchen
Copy link
Collaborator Author

@wsmoses All tests have passed. Two fails are due to other test cases

enzyme/test/Enzyme/ReverseMode/blas/cblas_daxpy_a.ll Outdated Show resolved Hide resolved
enzyme/Enzyme/AdjointGenerator.h Outdated Show resolved Hide resolved
enzyme/Enzyme/AdjointGenerator.h Outdated Show resolved Hide resolved
enzyme/Enzyme/AdjointGenerator.h Outdated Show resolved Hide resolved
enzyme/Enzyme/AdjointGenerator.h Outdated Show resolved Hide resolved
enzyme/Enzyme/AdjointGenerator.h Outdated Show resolved Hide resolved
enzyme/Enzyme/AdjointGenerator.h Outdated Show resolved Hide resolved
enzyme/Enzyme/AdjointGenerator.h Outdated Show resolved Hide resolved
enzyme/Enzyme/AdjointGenerator.h Outdated Show resolved Hide resolved
enzyme/test/Enzyme/ReverseMode/blas/cblas_daxpy_x.ll Outdated Show resolved Hide resolved
@vchuravy
Copy link
Member

@tansongchen you marked all the conversations as resolved, but there is no new commit yet? Maybe you forgotten to push?

@tansongchen tansongchen changed the title Add cblas level 1 and 2 routines and corresponding regression tests Add cblas level 1 routines and corresponding regression tests Jan 31, 2022
@tansongchen
Copy link
Collaborator Author

@tansongchen you marked all the conversations as resolved, but there is no new commit yet? Maybe you forgotten to push?

Just pushed

@tansongchen tansongchen force-pushed the cblas_sscal branch 2 times, most recently from b0af1e5 to f82c4ca Compare February 14, 2022 20:59
@tansongchen
Copy link
Collaborator Author

I haven't quite understood what is happening with the Julia nightly CI. It seems that other PRs are also failing on this?

@wsmoses
Copy link
Member

wsmoses commented Feb 20, 2022

Yeah feel free to ignore Julia nightly CI.

getReverseBuilder(Builder2);
if (has_diff_x) {
#if LLVM_VERSION_MAJOR >= 11
auto swapCall =
Copy link
Member

Choose a reason for hiding this comment

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

Can you add to differentialuseanalysis.h a corresponding note that neither primal x, nor primal y are needed for the reverse pass here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which function should I add to? and is there some example to see how to add them

@wsmoses
Copy link
Member

wsmoses commented Feb 20, 2022

Modulo the rebase on #528 and potentially the pass-by-reference handling required this looks good!

Also we're going to want to list the blas routines behaviors in differential use analysis and activity analysis, since in theory we should be able to have stronger information than a generic call fallback.

@vchuravy
Copy link
Member

bump! It would be really good to get this done. This is the biggest blocker for Enzyme.jl currently

@tansongchen
Copy link
Collaborator Author

Let's get this merged step-by-step. I am firstly adding swap and copy functions here, and then the remaining level 1 functions, and then level 2, and then level 3. A helper function is implemented to handle the case where we swap a active vector with a constant vector.

@wsmoses
Copy link
Member

wsmoses commented Sep 27, 2023

Reopening to preseve tests which we should add cc @ZuseZ4

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.

None yet

3 participants