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

[FP16] Implement load and store instructions. #6796

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

brendandahl
Copy link
Collaborator

@brendandahl brendandahl requested review from kripken and tlively and removed request for kripken and tlively July 31, 2024 18:31
@brendandahl
Copy link
Collaborator Author

brendandahl commented Jul 31, 2024

I for some reason thought we used LLVM to build everywhere. I'll need to come up with some that that works for gcc/msvc.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Nice, looks good besides the build issues with the interpreter.

@@ -0,0 +1,11 @@
The MIT License (MIT)
Copy link
Member

Choose a reason for hiding this comment

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

This will be the first non-Apache2-licensed code (that is not testing code), so it might be worth a mention in our toplevel LICENSE file.

Any chance there's an Apache2-licensed alternative?

Copy link
Member

Choose a reason for hiding this comment

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

We could also reach out to Marat and ask if he's willing and able to dual-license it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A quick search didn't find any apache license libraries, only another MIT one. IANAL, but I was under the impression MIT is fine to include in apache.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't think it's a blocker to use this code, it is compatible AFAIU. But it is a different license so it seems worth mentioning at the top level.

@brendandahl
Copy link
Collaborator Author

Hmm...looks like the FP16 library is having issues with MSVC still.

@brendandahl
Copy link
Collaborator Author

I've opened Maratyszcza/FP16#31 for MSVC issue.

@brendandahl brendandahl merged commit 0c26948 into WebAssembly:main Aug 6, 2024
13 checks passed
@osa1
Copy link
Contributor

osa1 commented Aug 8, 2024

It looks like the conditionals in bitcasts.h are not working on some compiler/targets, I get these errors when building on Dart's Windows builders:

../../third_party/binaryen/src/third_party/FP16/include/fp16/bitcasts.h(26,9): error: use of undeclared identifier '_castu32_f32'
   26 |         return _castu32_f32(w);
      |                ^
../../third_party/binaryen/src/third_party/FP16/include/fp16/bitcasts.h(44,9): error: use of undeclared identifier '_castf32_u32'
   44 |         return _castf32_u32(f);
      |                ^
../../third_party/binaryen/src/third_party/FP16/include/fp16/bitcasts.h(62,9): error: use of undeclared identifier '_castu64_f64'
   62 |         return _castu64_f64(w);
      |                ^
../../third_party/binaryen/src/third_party/FP16/include/fp16/bitcasts.h(80,9): error: use of undeclared identifier '_castf64_u64'
   80 |         return _castf64_u64(f);
      |                ^
Note: including file:      ../../third_party/binaryen/src/third_party/FP16/include/fp16/macros.h

The logs don't include the compiler version, I'm trying to find the compiler used in this build. I will update.

@brendandahl
Copy link
Collaborator Author

@osa1 were you able to find what compiler version you are using?

@osa1
Copy link
Contributor

osa1 commented Aug 13, 2024

@brendandahl version 19.

@mkustermann
Copy link
Contributor

../../third_party/binaryen/src/third_party/FP16/include/fp16/bitcasts.h(26,9): error: use of undeclared identifier
'_castu32_f32'
26 | return _castu32_f32(w);

Binaryen seems to include immintrin.h in binaryen/third_party/FP16/include/fp16/bitcasts.h

#if defined(__INTEL_COMPILER) || defined(_MSC_VER) && (_MSC_VER >= 1932) && (defined(_M_IX86) || defined(_M_X64))
	#include <immintrin.h>
#endif

#if defined(_MSC_VER) && !defined(__clang__) && (defined(_M_ARM) || defined(_M_ARM64))
	#include <intrin.h>
#endif

static inline float fp32_from_bits(uint32_t w) {
  ...
#elif defined(__INTEL_COMPILER) || defined(_MSC_VER) && (_MSC_VER >= 1932) && (defined(_M_IX86) || defined(_M_X64))
	return _castu32_f32(w);   // <-- COMPILATION ERROR, _castu32_f32 not found.
...
#endif
}

(side note: Clang windows compiler will define _MSC_VER variables)

The clang windows compiler seems to ship this function

=> So I think the conditional include should be fixed to #include <intrin.h> instead of #include <immintrin.h>

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.

5 participants