WIP: wasm: New backend #160
No reviewers
Labels
No labels
Compat/Breaking
Frontend/ada
Frontend/c
Frontend/c++
Frontend/rust
General/forge
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Library/libgcc
Library/libstdc++
Midend/gimple
Midend/rtl
Midend/tree
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Target/aarch64
Target/arm
Target/i386
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No assignees
6 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gcc/gcc!160
Loading…
Reference in a new issue
No description provided.
Delete branch "feedable/gcc-TEST:gcc-wasm-2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I would like to submit my work on a GCC backend for WebAssembly for review and potential inclusion into mainline GCC.
WebAssembly is a target intended primarily for running in a web environment. It's very similar to NVPTX in that it is a virtual ISA with infinite registers intended to be further processed by a target runtime into target-specific machine code to be executed. WebAssembly differs, however, in that can be directly interpreted too, and there is an abundance of interpreters for it already.
The primary objective of WebAssembly is to be a target that is executable by a web browser in the same context as JavaScript. This allows people to write web applications in C or C++ (or any other compiled language). Now, however, it is also used in many other niches, mostly whenever there is a need for platform-agnostic execution environment that is isolated from critical parts of a program.
The port in its current state passes all but 80 tests in the GCC test suite, sans tests for debug info, since debug info output is not supported currently. The port uses an external assembler, linker, and C standard library implementation.
Please take a look at https://gcc.gnu.org/wiki/WebAssemblyBackend for more information.
Hi @feedable. This bot helps send your PR as a patch series to the mailing list.
Next steps
CC: Reviewer1 <revi.ewer@example.com>, Reviewer2 <an.other@example.com>./preview./submit.Changes since vXsection, and comment with/submitagain.See Submitting Patches and UsingForge.
Consider joining the gcc and gcc-patches mailing lists..
For real time communication, check the gcc irc channels.
Or join
#overseerson Libera Chat, particularly if this automation is not working (stay online to get replies, IRC does not save messages if people are not online).8976050eabb114c074bc@ -609,6 +609,10 @@ tic6x-*-*)extra_headers="c6x_intrinsics.h"extra_options="${extra_options} c6x/c6x-tables.opt";;wasm*-*-*)indendention is different from the others in this file.
@ -3674,2 +3678,4 @@tmake_file="visium/t-visium visium/t-crtstuff";;wasm*-*-*)target_has_targetm_common=notarget_has_targetm_common belongs to the first wasm case.
I think you are missing setting tmake_file.
I think I don't need that since I'm using the default one.
@ -0,0 +1,84 @@(define_c_enum "unspecv" [MIssing a copyright notice in front of the file.
The convention is to put the iterators in iterators.md.
@ -0,0 +21,4 @@#define IN_TARGET_CODE 1#include <limits>#include <utility>#include is already done in system.h
So is limits; well limits.h is done.
This was for numeric limits (for that template specifically), but it looks like it's no longer required.
@ -0,0 +92,4 @@is_escape (char x){const char *escapes = "\"\'\\\t\n\r";while (*escapes and x != *escapes)&& is used instead of and.
@ -0,0 +188,4 @@voidprint_type (FILE *stream, const_tree type, bool first = false)Missing comment saying what the functions does.
@ -0,0 +200,4 @@case NULLPTR_TYPE:case VECTOR_TYPE:case COMPLEX_TYPE:print_type (stream, ptr_type_node, first);This seems wrong for vector and complex types.
This is actually dead code, so I removed it
@ -0,0 +217,4 @@case INTEGER_TYPE:case BOOLEAN_TYPE:case ENUMERAL_TYPE:fprintf (stream, "%s%s", delim,This seems wrong.
And does not handle BITINT.
BITINT is unsupported for now. This function prints the wasm's PoV on the type (as if it's in function signatures etc), so even for small ints it will be a full i32. What else is wrong?
@ -0,0 +311,4 @@if (VOID_TYPE_P (type))return;if (TREE_CODE (type) == COMPLEX_TYPE&& targetm.calls.split_complex_arg (type))Since you are in target code directly and I doubt you have defined a split_complex_arg target hook, this becomes reachable.
The hook is defined to false, since LLVM ABI wants them to be in memory, so the code is really dead, but it would be nice if that would adapt if it ever changes
@ -0,0 +432,4 @@constexpr int ndata = sizeof (T) / 4;long data[ndata];real_to_target (data, n, float_mode_for_size (sizeof (T) * 8).require ());int mantissa = std::numeric_limits<T>::digits - 1;Using the host floating point mode here seems wrong.
to get the number of significiant bits use significand_size with format_helper.
@ -0,0 +672,4 @@voidwasm_assemble_decl_end (){return decl_end(asm_out_file);Missing space between the function name and
(.@ -0,0 +1,12 @@Missing copyright notice.
@ -0,0 +1,125 @@#include "config.h"Missing copyright notice.
@ -0,0 +164,4 @@#define BITS_BIG_ENDIAN 1#define BYTES_BIG_ENDIAN 0#define WORDS_BIG_ENDIAN 0This seems odd to define BITS big endian to 1 without the others.
Is it wasm really
most significant bit is lowest numbered?@ -0,0 +165,4 @@#define BITS_BIG_ENDIAN 1#define BYTES_BIG_ENDIAN 0#define WORDS_BIG_ENDIAN 0#define UNITS_PER_WORD 8So the underlying registers are 64bit?
But then this is a ILP32 target?
Registers come in 2 distinct classes: 32- and 64-bit. 64-bit registers are used for passing
long longargs, so I have to support that too. Pointers are 32-bit though.@ -0,0 +182,4 @@#define WCHAR_TYPE_SIZE 32/* ??? Need to figure out why do we have to set this in order for TI to notappear */#define MAX_FIXED_MODE_SIZE 64MAX_FIXED_MODE_SIZE defaults to
MAX (BITS_PER_WORD * 2, 64)asBITS_PER_WORDis defined asUNITS_PER_WORD*BITS_PER_WORDmeaning the max will be 128. So defining it to 64 is correct if you don't support 128bit (yet).Yes, that's right, I don't support that
@feedable wrote in gcc/gcc-TEST#160 (comment):
Right I was trying to explain why you need to define the macro since you had a
???there.Oh right, got it. I can remove the comment, then.
@ -0,0 +6,4 @@"(nop)"[])(define_predicate "symbol_operand"The convention for new targets is to place predicates in predicates.md.
@ -1,3 +1,5 @@/* { dg-skip-if "wasi-libc doesn't work well with analyzer" { wasm*-*-* } } */It might make sense to xfail this rather than skip. Depending on the failure mode. The analyzer has special code to handle errno so it might need to be updated to support wasi-libc here.
I think it's the other way around, and wasi-libc doesn't have the attributes required to say that errno is errno and not some other thing, if I understand correctly
@feedable wrote in gcc/gcc-TEST#160 (comment):
No analyzer code deals with errno as a special case; there is no attribute for errno. See analyzer/kf.cc:3184 . It might just need to add support for the wasi-libc case here.
It looks like wasi-libc just uses a thread-local variable by the name
errnofor that:github.com/WebAssembly/wasi-libc@04bd527eb3/libc-bottom-half/headers/public/__errno.h (L8), but I don't feel like markingerrnoas a special location here would be a great idea.I would xfail for now and have a bug report ready to file when the code is merged or ask David M. for help on getting the right thing hooked up.
@ -0,0 +1,56 @@/* { dg-additional-options "-std=gnu17" } */Is there a reason why you need -std=gnu17 for this testcase?
It was in the original test (pr67037.c), so I kept it.
@ -3,6 +3,7 @@attribute alloc_size that __builtin_object_size can make use of (orare treated as if they were for that purpose)..{ dg-do compile }{ dg-skip-if "wasm uses wasi-libc" { wasm*-*-* } }This skip message does not makes sense since the testcase just uses all
__builtin_*functions. So there is no dependency on the libc. If anything maybe it should be an xfail.@ -5,2 +5,3 @@/* { dg-do run } *//* { dg-do run }{ dg-skip-if "wasm does not support prefetch" { wasm*-*-* } } */if a target does not provide a prefetch pattern, then the testcase should still work as the expansion of the prefetch builtin turns into no RTL instruction. So this skip seems wrong.
It does work, but it warns every time that prefetch is not meanigful. I don't want to silence that warning by making a dummy pattern, so skip. I think another option would be to expect warnings everywhere, ig?
Hmm, I don't see how this would warn.
expand_builtin_prefetch does not call warning for the
!targetm.have_prefetch ()case. This sems like something else is going wrong.Yep, this doesn't even seem to fail now.
@ -1,6 +1,7 @@/* PR c/17308 - nonnull attribute not as useful as it could bePR c/78673 - sprintf missing attribute nonnull on destination argument{ dg-do "compile" }{ dg-skip-if "wasm uses wasi-libc" { wasm*-*-* } }Skip message seems wrong as this is a compile time testcase without dependancy on the libc.
Maybe a xfail or is this due to variable argument printf?
This appears to not be failing anymore. The message also doesn't really make sense now. Deleted.
@ -1,6 +1,7 @@/* Test __STDC_VERSION_STDINT_H__ in C23. *//* { dg-do preprocess } *//* { dg-options "-std=c23 -pedantic-errors -ffreestanding" } *//* { dg-skip-if "wasi-libc doesn't support c23" { wasm*-*-* } } */You should be using a stdint wrapper then. Look at other examples in config.gcc for that.
@ -1,5 +1,5 @@/* { dg-do compile } *//* { dg-skip-if "only works for ELF targets" { *-*-darwin* *-*-aix* } } *//* { dg-skip-if "only works for ELF targets" { *-*-darwin* *-*-aix* wasm*-*-* } } */This should be changed to
{ elf }and submitted seperately.Made a separate patch for it. I want to keep in the series though, since I think the set will grow during review and I don't want to spam small patches in the meantime, so I hope that's fine with you.
We get many small patches. A few more won't hurt and getting this out of the way makes reviewing the main parts of the patch easier, IMO.
Version 1 of this pull request has been stored. It includes the following commits:
184da0c3e9096aa4e09295bbe94cdcb114c074bcb114c074bcdad99c1cb4dad99c1cb4762c13c12a762c13c12a276f248c12276f248c122eb0a7b9492eb0a7b949f270ac3975f270ac3975ac20dcd5f8Version 2 of this pull request has been stored. It includes the following commits:
771a580b4e64d9153aa373c55272238f90eb001aac20dcd5f8Pull Request versions:
6e1679ebd5b114c074bc33ac889aa2ac20dcd5f8In order to compare , clone this repository and run
@ -0,0 +44,4 @@(define_mode_attr promote_type [(QI "i32") (HI "i32")(SI "i32") (DI "i64")(SF "f32") (DF "f32")])I don't understand why this doesn't say
(DF "f64"). Is this a special case?Nope, this is just an oversight. Didn't catch that myself because only integer types here are actually used.
@ -0,0 +174,4 @@#define FUNCTION_MODE SImode#define CASE_VECTOR_MODE SImode#define POINTER_SIZE 32Are you trying to follow the https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md document?
I think there are some inconsistencies with that:
long double is emitted as 8-byte DFmode, TImode is disabled, plain char is
unsigned, and wchar_t is long. The WebAssembly Basic C ABI requires
binary128 long double, 128-bit integers as two i64s, signed plain char, and
wchar_t as int.
I can't really enable
TImodenow (or anything 128-bit), since to my understanding there isn't machinery to split these values to registers of appropriate size, and that's only likely to change when I make the register allocation infrastructure work with infinite registers. I would also then want to disablelong doublefor now, since that's also subject to the same problems. The rest is fair, will fix@ -0,0 +585,4 @@return wasm_function_value (rtype, NULL_TREE, false);}/* Implementation of TARGET_PASS_BY_REFERENCE. */@ -0,0 +98,4 @@}voidrecord_libcall (const_rtx sym, tree ret)What do you mean by "ordinary structs" here?
aggregate_value_palready handles aggregate types that are not transparent (i.e. have a single field of scalar type) and always returns them by reference, sowasm_return_in_memorydoesn't even encounter them.The one thing that's not handled are empty structs, which didn't manifest because
TARGET_EMPTY_RECORD_Pis false by default.@ -3756,6 +3756,9 @@ assign_parms (tree fndecl)targetm.calls.function_arg_advance (all.args_so_far, data.arg);}targetm.calls.function_incoming_arg (all.args_so_far,Yes, but isn't that the point? My impression was that
end_markerwas the canonical way to indicate the end of function arguments. If we actually don't want that (or can't afford to change the targets that rely on the current behavior), I can make another hook for this place specifically, instead, too.@ -0,0 +235,4 @@[(set (match_operand:SUBREGDI 0 "subregister_for_di_operand")(match_operand:SUBREGDI 1 "memory_operand"))]"""(%o0 (i64.load<SUBREGSI:size>_s %m1))")I think this should be SUBREGDI:size as that's what the pattern iterator is using
View command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.