This is an archive of the discontinued LLVM Phabricator instance.

Differential D138760

BLAKE3: do not try to use neon on big-endian aarch64
AbandonedPublic

Authored by MaskRay on Nov 27 2022, 7:46 AM.

Details

Reviewers
akyrtzi
dexonsmith
gkistanova
he32
Summary

...because this will later on hit

#error "This implementation only supports little-endian ARM."

in blake3_neon.c.

Diff Detail

Event Timeline

he32 created this revision.Nov 27 2022, 7:46 AM
Herald added a reviewer: gkistanova. · View Herald TranscriptNov 27 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: hiraditya, kristof.beyls. · View Herald Transcript
he32 requested review of this revision.Nov 27 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2022, 7:46 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
gkistanova resigned from this revision.Nov 29 2022, 10:25 AM

Doesn't look like I'm a good reviewer for the proposed patch.

he32 added a comment.EditedDec 17 2022, 5:20 AM

Last reviewer resigned, despite this being a fairly trivial change which blocks big-endian arm64 from building.
Can someone else please take a look at reviewing this trivial change?
(I did not find a suitable action to do this automatically, and am not comfortable pointing to anyone in specific...)

he32 edited reviewers, added: krytarowski; removed: gkistanova.Dec 27 2022, 2:42 PM

Could you please take a look at this rather simple diff, fixing the build on BE aarch64.

Herald added a reviewer: gkistanova. · View Herald TranscriptDec 27 2022, 2:42 PM
efriedma edited reviewers, added: akyrtzi, MaskRay, dexonsmith; removed: krytarowski, gkistanova.Dec 27 2022, 2:54 PM
Herald added a reviewer: gkistanova. · View Herald TranscriptDec 27 2022, 2:54 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
MaskRay added a comment.Dec 27 2022, 3:46 PM

Can you report the problem on https://github.com/BLAKE3-team/BLAKE3 ? We can back port fixes there.

he32 added a comment.Dec 28 2022, 5:16 AM

Thanks for the hint / redirect.
I've submitted a pull request at https://github.com/BLAKE3-team/BLAKE3/pull/280

MaskRay added a comment.Dec 30 2022, 10:26 AM
In D138760#4018383, @he32 wrote:

Thanks for the hint / redirect.
I've submitted a pull request at https://github.com/BLAKE3-team/BLAKE3/pull/280

Thank you! If we take the patch here, we may risk losing the change when someone upgrades BLAKE3. So it's better to ensure that it works in the upstream...

dexonsmith added a comment.Dec 30 2022, 11:15 AM
In D138760#4020385, @MaskRay wrote:
In D138760#4018383, @he32 wrote:

Thanks for the hint / redirect.
I've submitted a pull request at https://github.com/BLAKE3-team/BLAKE3/pull/280

Thank you! If we take the patch here, we may risk losing the change when someone upgrades BLAKE3. So it's better to ensure that it works in the upstream...

Can’t we have a test/bot for that? Seems awkward to depend on this getting integrated upstream first. This is just unblocking builds so I’d be inclined to take it…

MaskRay commandeered this revision.Sep 19 2023, 5:30 PM
MaskRay edited reviewers, added: he32; removed: MaskRay.

Obsoleted by D159156. Upstream BLAKE3 has applied this diff https://github.com/BLAKE3-team/BLAKE3/pull/280

MaskRay abandoned this revision.Sep 19 2023, 5:30 PM

Revision Contents

PathSize
llvm/
lib/
Support/
BLAKE3/
6 lines

Diff 478100

llvm/lib/Support/BLAKE3/blake3_impl.h

Context not available.
#if !defined(BLAKE3_USE_NEON) #if !defined(BLAKE3_USE_NEON)
// If BLAKE3_USE_NEON not manually set, autodetect based on AArch64ness // If BLAKE3_USE_NEON not manually set, autodetect based on AArch64ness
#if defined(IS_AARCH64) #if defined(IS_AARCH64)
#define BLAKE3_USE_NEON 1 #if defined(__ARM_BIG_ENDIAN)
#define BLAKE3_USE_NEON 0
#else
#define BLAKE3_USE_NEON 1
#endif
#else #else
#define BLAKE3_USE_NEON 0 #define BLAKE3_USE_NEON 0
#endif #endif
Context not available.