Skip to content

Conversation

@brendandahl
Copy link
Collaborator

Ensure the "fp16" feature is enabled for FP16 instructions.

@brendandahl brendandahl requested review from kripken and tlively August 21, 2024 22:27
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.

We should also update fuzzing.cpp to gate generation of the fp16 ops behind the feature flag.

const char* MultiMemoryFeature = "multimemory";
const char* TypedContinuationsFeature = "typed-continuations";
const char* SharedEverythingFeature = "shared-everything";
const char* FP16Feature = "fp16";
Copy link
Member

Choose a reason for hiding this comment

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

Just to double check, does this match the feature name used by LLVM in the custom features section?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will after llvm/llvm-project#105434

@brendandahl
Copy link
Collaborator Author

We should also update fuzzing.cpp to gate generation of the fp16 ops behind the feature flag.

Preference for in this PR or follow up?

@tlively
Copy link
Member

tlively commented Aug 22, 2024

Ideally in this one. Thanks!

Ensure the "fp16" feature is enabled for FP16 instructions.
@brendandahl
Copy link
Collaborator Author

Added some fuzzing. FWW, I noticed we're missing relaxed simd support and splat instructions for floats.

@brendandahl brendandahl requested a review from tlively August 22, 2024 17:39
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm with fuzzing.

Side note, for incremental updates to a PR I would ask that we all try to use additional commits, rather than force-pushing a single commit. Incremental commits make incremental review easier, at least for me.

@brendandahl
Copy link
Collaborator Author

I try not to force push if I've rebased in upstream, but for small commits it seems the github "compare" link on the force pushes is as good as a separate commit.

@brendandahl
Copy link
Collaborator Author

I guess the review UI is not on the compare link. I'll try to stick with separate commits.

@brendandahl brendandahl merged commit 95a280f into WebAssembly:main Aug 22, 2024
@kripken
Copy link
Member

kripken commented Aug 22, 2024

I wasn't aware of the "compare" button, thanks! Looks useful.

But, yeah, it does seem to be missing the review UI, unfortunately, which commits do have.

@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

3 participants