Skip to content

Conversation

SanftMonster
Copy link
Collaborator

@SanftMonster SanftMonster commented Nov 8, 2023

Just for review, please don't merge it.

@SanftMonster SanftMonster mentioned this pull request Nov 8, 2023
5 tasks
SignalRT added a commit to SignalRT/LLamaSharp that referenced this pull request Nov 8, 2023
Just merge cuda and avx detection and change layout.
if(Avx512F.IsSupported) level = AvxLevel.Avx512;
#endif
?? TryLoad("avx2/libllama.so", System.Runtime.Intrinsics.X86.Avx2.IsSupported)
?? TryLoad("avx/libllama.so", System.Runtime.Intrinsics.X86.Avx.IsSupported)
Copy link
Member

Choose a reason for hiding this comment

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

AVX/AVX2/AVX512 detection should already be handled by this

// Try to load a preferred library, based on CPU feature detection
TryLoadLibrary();
#if NET6_0_OR_GREATER
NativeLibrary.SetDllImportResolver(typeof(NativeApi).Assembly, LLamaImportResolver);
Copy link
Member

Choose a reason for hiding this comment

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

I deliberately avoided using NativeLibrary.SetDllImportResolver when I originally set this up, since it overwrites global configuration that may already be in use by the application.

@martindevans
Copy link
Member

The intention when I originally introduced this was that new dependencies could easily be added (in order of preference, most preferred first) something like this:

return TryLoad("magic/libllama.dll", IsMagicSupported())
       ?? TryLoad("unicorns/libllama.dll", IsUnicornsSupported())
       ?? TryLoad("rainbows/libllama.dll", IsRainbowsSupported())
       ?? etc...

i.e. any new platform can simply be chained in, so long as you can write a boolean method that prechecks if that particular thing is supported.

I deliberately avoided using NativeLibrary.SetDllImportResolver since in theory it could have already been set externally on this assembly, and that would cause an exception the second time. I guess it's not a huge risk really, it just feels like a poorly designed API really (feels like it should be chainable).

SignalRT added a commit to SignalRT/LLamaSharp that referenced this pull request Nov 9, 2023
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