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

[BLIS] Update to v1.0 #8626

Merged
merged 3 commits into from
May 27, 2024
Merged

Conversation

jd-foster
Copy link
Contributor

Update BLIS library using master branch, but simultaneous with release point of version 1.0.0: https://github.com/flame/blis/releases/tag/1.0

As part of the update process, updated/omitted some of the more outdated patches.

Tested locally on x86_64-apple-darwin against Julia LinearAlgebra blas tests.

@imciner2
Copy link
Member

imciner2 commented May 7, 2024

Can you verify that libblastrampoline still sees all the functions in 1.0 properly? (e.g. does it have the same number forwarded)?

@jd-foster
Copy link
Contributor Author

jd-foster commented May 7, 2024

The information coming from

LinearAlgebra.BLAS.lbt_forward(blis_jll.blis; clear=true, verbose=true, suffix_hint="64_")

is

Generating forwards to /Users/~/.julia/artifacts/5ed1d60e5743e3c78830b75aa34d0f32e8719905/lib/libblis.4.0.0.dylib (clear: 1, verbose: 1, suffix_hint: '64_')
 -> Autodetected symbol suffix "64_"
 -> Autodetected interface ILP64 (64-bit)
 -> Autodetected normal complex return style
 -> Autodetected gfortran calling convention
 -> Autodetected CBLAS-divergent library!
 - [2547] cblas(cblas_cdotc_sub64_)
 - [2549] cblas(cblas_cdotu_sub64_)
 - [2695] cblas(cblas_zdotc_sub64_)
 - [2697] cblas(cblas_zdotu_sub64_)
 - [2588] cblas(cblas_ddot64_)
 - [2655] cblas(cblas_sdot64_)
Processed 4949 symbols; forwarded 167 symbols with 64-bit interface and mangling to a suffix of "64_"
167

This is slightly more than previous versions of blis_jll1 now that flame/blis#778 was merged.

@imciner2
Copy link
Member

imciner2 commented May 7, 2024

Great.

Can you also touch/modify the build_tarball files in the blis and blis32 directories to trigger the new build of the library here? We only rebuild when those files are changed, and this only touched the common file, so it didn't see anything to build.

Comment on lines -90 to -93
patch kernels/armsve/1m/old/bli_dpackm_armsve512_int_12xk.c \
< ${WORKSPACE}/srcdir/patches/armsve_kernels_unscreen_arm_sve_h.patch
patch kernels/armsve/1m/bli_dpackm_armsve256_int_8xk.c \
< ${WORKSPACE}/srcdir/patches/armsve_kernels_unscreen_arm_sve_h.patch
Copy link
Member

Choose a reason for hiding this comment

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

Why are these removed? If they are unneeded, should the corresponding file be removed? But I hope we aren't missing optimisations on ARM SVE architectures.

Copy link
Contributor Author

@jd-foster jd-foster May 7, 2024

Choose a reason for hiding this comment

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

The second (bli_dpackm_armsve256_int_8xk.c) doesn't exist anymore after a refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However the applied armsve_kernels_unscreen_arm_sve_h.patch is still used in the patch in line 89.

Copy link
Member

Choose a reason for hiding this comment

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

The bli_dpackm_armsve512_int_12xk still appears to have the !defined(BLIS_FAMILY_A64FX) in it, but is that actually used anywhere? A search in the BLIS code base for the symbol in that kernel (bli_dpackm_armsve512_int_12xk) does not show any other uses.

The bli_dpackm_armsve256_int_8xk.c appears to have been changed, there is only a bli_dpackm_armsve256_int_8x10.c now, but it does have the !defined(BLIS_FAMILY_A64FX) guarding it, however that kernel is actually also only selected part of the time, since it is configured with this case (https://github.com/flame/blis/blob/6d0ab74f6975fdf4d19cee06d946b09b6ca89656/kernels/armsve/bli_kernels_armsve.h#L47C1-L51C57):

// Use SVE intrinsics only for referred cases.
#if !defined(BLIS_FAMILY_A64FX)
PACKM_KER_PROT( double,   d, packm_armsve256_int_8x10 )
#endif
PACKM_KER_PROT( double,   d, packm_armsve512_asm_16x10 )

So, will this still be a problem? It looks like they have an SVE 512 assembly kernel for the A64FX processor and so won't be using that SVE 256 kernel there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first (patch kernels/armsve/1m/old/bli_dpackm_armsve512_int_12xk.c) seems to be unused in the code anyway.
cf. flame/blis@2604f40

Copy link
Contributor Author

@jd-foster jd-foster May 7, 2024

Choose a reason for hiding this comment

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

@xrq-phys Do you have insights here on the appropriate ARM SVE optimisations?

@jd-foster
Copy link
Contributor Author

Can you also touch/modify the build_tarball files in the blis and blis32 directories to trigger the new build of the library here?

Thanks (not the first time I've forgotten that step!).

@jd-foster
Copy link
Contributor Author

jd-foster commented May 7, 2024

I've restored the patches using the same pattern of #if !defined(BLIS_FAMILY_A64FX) to #if 1.

The patching tested locally fine but ... clearly I don't know how to do patches (on other platforms and generally).

Help would be greatly appreciated with getting atomic_patch working here.

@jd-foster jd-foster force-pushed the blis-v1-release branch 2 times, most recently from b5f54fc to 8ed2579 Compare May 8, 2024 05:02
@jd-foster
Copy link
Contributor Author

Ok, figured out the patching, and usually helps to set the right commit hash :). This looks better.

@jd-foster
Copy link
Contributor Author

The patches now cover the effective function of the ARM SVE patches put in previously by @xrq-phys , I believe, taking account of the refactored code.

So I think we're good to go with this?

Co-authored-by: Mosè Giordano <[email protected]>
@imciner2
Copy link
Member

Looks fine to me now,

Has there been any communication with upstream about these SVE patches at all? It would be good to get them either upstreamed or clarified by the upstream developers so we don't have to keep carrying the patches.

@jd-foster
Copy link
Contributor Author

Has there been any communication with upstream about these SVE patches at all? It would be good to get them either upstreamed or clarified by the upstream developers so we don't have to keep carrying the patches.

I opened an issue here: flame/blis#807

@imciner2
Copy link
Member

@giordano any comments on this, or can we merge this now that the ARM patches have been added back and upstream notified about them?

@giordano giordano merged commit 2e85452 into JuliaPackaging:master May 27, 2024
19 checks passed
amontoison pushed a commit that referenced this pull request Jun 22, 2024
* Testing pre-v1.0 release on master

* Update to version 1.0.0 using master branch at release (second try), fix patching

* Update build_tarballs.jl

Co-authored-by: Mosè Giordano <[email protected]>

---------

Co-authored-by: Mosè Giordano <[email protected]>
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.

3 participants