This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add multiple memories feature
ClosedPublic

Authored by ashleynh on Aug 21 2023, 4:31 AM.

Details

Summary

Adding to allow users to get this flag into the target features section for future use cases.

Diff Detail

Event Timeline

ashleynh created this revision.Aug 21 2023, 4:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 4:31 AM
ashleynh requested review of this revision.Aug 21 2023, 4:31 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 21 2023, 4:31 AM

The other file to update here is clang/test/Driver/wasm-features.c. It looks like we haven't been as consistent about updating that one.

clang/include/clang/Driver/Options.td
4586–4587

Can we call this "multimemory" for consistency with "multivalue" above?

sbc100 added inline comments.Aug 21 2023, 8:50 AM
clang/include/clang/Driver/Options.td
4586

How about just multi_memory?

In the past we have talked about "multi-table" and "multi-memory" without using the plural here and the proposal itself is names using the singular (https://github.com/WebAssembly/multi-memory).

The other file to update here is clang/test/Driver/wasm-features.c. It looks like we haven't been as consistent about updating that one.

Darn it, you beat be by 2 minutes.

ashleynh updated this revision to Diff 552071.Aug 21 2023, 10:27 AM

Addressing comments
Renamed flag, test file, and added to wasm-features.c

ashleynh updated this revision to Diff 552088.Aug 21 2023, 11:35 AM

Squashed into one commit

aheejin accepted this revision.Aug 21 2023, 12:04 PM

LGTM % nit and the name resolution (multimemory vs. multi-memory)

clang/include/clang/Driver/Options.td
4586–4587

I like multi-memory more, but I preferred multi-value too when it was introduced...

llvm/lib/Target/WebAssembly/WebAssembly.td
76

Other feature descriptions are also in plain sentences, so..

This revision is now accepted and ready to land.Aug 21 2023, 12:04 PM
aheejin added inline comments.Aug 21 2023, 12:06 PM
clang/include/clang/Driver/Options.td
4586–4587

But if we are going to remove - here, we should treat it consistently in all other places, for example, it should be not HasMultiMemory but HasMultimemory in all code. We treat multivalue that way. Do we want that?

ashleynh updated this revision to Diff 552097.Aug 21 2023, 12:10 PM

switching back to multiple memories for description

tlively added inline comments.Aug 21 2023, 12:12 PM
clang/include/clang/Driver/Options.td
4586–4587

I prefer HasMultiMemory fwiw 😂

tlively accepted this revision.Aug 21 2023, 12:24 PM
ashleynh added inline comments.Aug 21 2023, 2:12 PM
clang/include/clang/Driver/Options.td
4586–4587

I think multi-memory is more readable, but happy to do multimemory to fall in line with the other flags.

+1 to preferring HasMultiMemory for readability.

This revision was landed with ongoing or failed builds.Aug 21 2023, 2:23 PM
This revision was automatically updated to reflect the committed changes.