- User Since
- Nov 12 2013, 11:44 AM (257 w, 5 d)
Thu, Oct 18
Can we remove the optional altogether now, and just always do the fixup?
Wed, Oct 3
LGTM. In the future it's possible we could need to hold the signatures somewhere other than the AsmPrinter, however we can figure that out later, when it's more clear what the MC AsmParser layer will need.
Thu, Sep 27
Mon, Sep 24
Sep 21 2018
The prompt here is that the LLVM wasm backend will be leaving "experimental" status. That means in about 6 months, it'll be part of an LLVM release. Once that happens, it'll be harder to make changes like this.
Sep 20 2018
https://reviews.llvm.org/D50568 is now fixed!
Sep 4 2018
On one hand, clang-format -style=LLVM really isn't the one exclusive formatting style of LLVM. CodingStandards.rst repeatedly emphasizes this. When we're working in LLVM outside of wasm-specific code, we need to respect the existing code formatting which isn't always clang-formatted, or locally reformat it as needed when making changes, so it's not a bad idea for us to get used to working that way anyway. And we accept patches that aren't clang-formatted.
Aug 29 2018
I guess I don't have a strong opinion here. The code is already pretty close to clang-format's interpretation of LLVM style, so looking at the actual diffs you're right that it doesn't seem to be a big change. Changes like those in WebAssemblyLowerBrUnless.cpp are a little unfortunate, but tolerable.
The LLVM coding standards say "we explicitly do not want patches that do large-scale reformatting of existing code". They suggest that reformatting should be done to address specific readability issues or to prepare an area for subsequent changes.
Aug 22 2018
Aug 16 2018
This looks ok to land now.
Aug 8 2018
Is this related to the issue reported in the thread here?
Jul 24 2018
First, I apologize, I'm not sure how I lost track of this patch and didn't see the pings.
https://reviews.llvm.org/D40526 has now landed. Is there anything else we should do before leaving "experimental" status?
Jul 23 2018
Jul 20 2018
For the record, I'm opposed to having the WebAssembly backend translate asm volatile(""::: "memory") differently from other targets in LLVM or GCC, which to my knowledge always translate it to a no-op, even on platforms with hardware fence instructions.
Ah, that's useful context, thanks. That'd be good to mention in the commit message and/or comments somewhere. Also, please add a testcase which approximates that use case.
Jul 19 2018
Not all types are valid to cast to all other types. But beyond that, normal native platforms don't insert non-trivial casts in such situations, so even in many cases where it's valid to cast, it doesn't seem like programmers could reasonably expect a non-trivial cast to happen. What kinds of use cases are motivating this?
Jul 17 2018
I'm not deeply familiar with MC's Asm parsing support, but this looks like a good direction to go.
Jul 11 2018
Jul 10 2018
I've now refreshed the emscripten patches and am hoping they can land soon.
Indeed; after discussing this with a few other folks, I think this is getting close.
Jun 25 2018
My biggest concern here is that this pass shouldn't affect prototype-possessing functions, and that appears to be satisfied :-).
Jun 22 2018
I haven't thought through all the possibilities related to !FD->doesThisDeclarationHaveABody(), but overall this looks good.
May 30 2018
May 18 2018
This looks right to me, but just to make sure I understand, has this been broken since r228400?
Apr 9 2018
Apr 4 2018
Looks good to me. Thanks!
Mar 8 2018
This user-defined custom sections patch should be ready to go now.
Mar 2 2018
We want to coordinate landing this with corresponding changes in Emscripten. Since this is an ABI change, Emscripten needs to figure out how to manage it. See here for ongoing discussion.
Indeed; I don't anticipate LLVM will be using mem.grow or mem.size itself. But, clang has __builtin_wasm_mem_grow and __builtin_wasm_mem_size, to allow standard libraries to use them. It's desirable that the names of these builtins, and the corresponding LLVM IR intrinsics, match the names in the official WebAssembly documentation.
Feb 27 2018
We should probably also wait for the WebAssembly committee to settle on final instruction names.
Feb 23 2018
I'm not very familiar with the Emscripten parts of this, but some grepping in the emscripten tree found these lines:
src/preamble.js: env['memory'] = Module['wasmMemory']; src/preamble.js: env['memory'] = providedBuffer; src/preamble.js: assert(env['memory'] instanceof ArrayBuffer);
Feb 20 2018
Feb 15 2018
Feb 12 2018
Please restore the "-wasm" forms of these tests too.
The new tests look good, but it'd be good to keep testing "wasm32-unknown-unknown-wasm" and "wasm64-unknown-unknown-wasm" here too.
Feb 9 2018
I think it's because writeBytes is defined to be passed data from an MCFragment's getContents(), which is a SmallVectorImpl<char>. That's the case here too, though the code that does the getContents() is a little separated from the writeBytes. Following @ncw's suggestion, I created a struct type, which not only avoids repeating the type name, but also makes the use of it cleaner than std::pair.
Created a struct to replace the noisy std::pair code.
@sbc100 I've updated the patch to leave the "env" namespace in place. Do you think this is ready to land now?
There is also an upside to not supporting unprototyped functions. Not only did people writing C in the distant past before prototypes not know what we know today about what it takes to have reasonably robust C code, but leaving C code unmaintained for decades is the opposite of what it takes to have reasonably robust C code, so such code is in double trouble. Obliging people who have unmaintained C codebases to either maintain them or forego using them on new platforms is a positive outcome. It's within reason to assign a variety of relative weights to that outcome, relative to the other outcomes.
Add an MC test.
Feb 8 2018
Feb 7 2018
As an example, you can have code like this:
#ifdef __wasm__ // do stuff with __builtin_wasm_mem_grow and __builtin_wasm_mem_size #endif
which works for both wasm32 and wasm64.
In my experience it's not uncommon to see a reference to "x86" where it's not clear whether it applies to both 32-bit and 64-bit x86, or really just 32-bit x86. Also, I think many of the old architectures have names that predate the existence of their 64-bit variants. For example MIPS was originally 32-bit only. In contrast, RISC-V is an example of a new architecture with a 64-bit variant present from the start, and they call their 32-bit variant riscv32. Consequently, I've been trying to only say "wasm" for things that apply to both wasm32 and wasm64.
Feb 5 2018
- Go back to using "env" for now, so that this is independent of the symbol-table changes and avoids bike-shedding discussions for now.
- Split the clang changes out into a separate patch, which I'll post separately.
Feb 2 2018
I mostly agree. _end/__wasm_end seem reasonable for now. My preference would be to wait on _edata and __bss_start until we know how those might be used.
I'm nervous about adding all these symbols. I like ELF features when they make sense, but for some of these symbols here, it's difficult to think of even hypothetical valid use cases.
Is there a use for _edata? I'm curious how applications would use it differently from _end.
Jan 31 2018
Jan 26 2018
Is this fixing a bug? It would help me to see what kind of code triggers this bug.
Jan 25 2018
Jan 24 2018
Jan 23 2018
Rebase for upstream changes.
The codegen tests in LLVM all still use all 4 parts, and I tend to prefer that, because it avoids problems where tests pass on some hosts and fail on others due to LLVM completing the triple by auto-detecting things from the host it's running on. This may not affect wasm today, but it's a habit I'm in from other LLVM work. However, if there are reasons to omit some of the parts in the lld tests, I'm not opposed either.
To make sure I didn't miss anything, this is just removing the "-wasm" parts now that that's the default, right?
Makes sense. With the direction all the symbol-table things are headed, using the name section as a kind of secondary symbol name is indeed confusing.
Jan 17 2018
Looks good to me. Thanks for cleaning this up!
Jan 5 2018
Ah, I overlooked that the loop doesn't dominate the exit block.
Jan 4 2018
I haven't thought about this in depth, but it seems like SCEV theoretically should be able to cope under such circumstances.
Dec 20 2017
I'm behind on some of the discussions on this topic at the moment, but overall, this looks like it's headed in a good direction, so I'm in favor of this patch.
Dec 19 2017
Dec 14 2017
Dec 13 2017
Either way. I won't be able to get to it until next week, so feel free to land it earlier.
Omitting the start section part for now is ok with me. It should be sufficient to just remove the Priority == UINT16_MAX special cases.
Dec 11 2017
Yes, wasm has its own object format, so this change makes sense.
Dec 7 2017
This updates the patch to make size_t be unsigned long unconditionally, corresponding with the proposed patch to Emscripten here which changes it for asm.js as well.
The issue reported in PR35519 is now fixed, there's a testcase now, and I responded to @sanjoy's comment above.
Dec 6 2017
For the record, I believe that it would be better to work towards implementing the proposal here:
- Remove the WASM_DEFAULT_LINEAR_MEMORY and WASM_DEFAULT_FUNCTION_TABLE metadata.
- Update test/MC tests.
I'd like to probe the solution of importing the linear memory a little more. There's nothing in wasm that prevents syscalls from working in the wasm start function. The problem seems to just be pre-ES6-module JS. Non-Web, future-ES6-modules-Web, and future programs that are all wasm with no JS won't have this problem.
- Fixes debug info updating.
- Adds a proper testcase for the memset merging caching bug.
This adds a fix for the testcase reported in PR35519; see the change in MemCpyOptimizer.cpp. The code for merging stores into a memset wasn't properly updating memdep's query cache.
Dec 5 2017
Dec 4 2017
Ok, cool. Yeah, I was mainly thinking about --sysroot in clang.
Dec 3 2017
Sysroot is a useful feature on other platforms. What is the reason for removing it, rather than implementing it :-)?