- User Since
- Aug 31 2016, 4:27 PM (229 w, 2 d)
Sep 23 2019
Aug 27 2019
Landed as 92ed86d239cd
Reviewed in https://reviews.llvm.org/D65916
Aug 21 2019
Aug 12 2019
Overall direction looks good to me as well.
Aug 9 2019
- More precise help message
Aug 7 2019
Aug 1 2019
Feb 25 2019
Jan 28 2019
Seems good and simple, ship it
Nov 8 2018
Oct 3 2018
Forgot to add, need to add __attribute__((used, visibility("default"))) to btPersistentManifold::clearUserCache in that cpp file
Oct 2 2018
This still breaks two emscripten tests, test_the_bullet and test_poppler
In particular, it creates a wasm module that fails to typecheck, seemingly because of a reordering of instructions. Commenting out MoveDebugValues and CloneDebugValues is needed to avoid this.
The simplest reproduction I can find is:
wasm32-clang++ $EMSCRIPTEN/tests/bullet/src/BulletCollision/NarrowPhaseCollision/btPersistentManifold.cpp \ -D__EMSCRIPTEN__=1 \ -I$EMSCRIPTEN/tests/bullet/src/ \ -nostdlib \ -Xclang -nostdsysteminc \ -Xclang -isystem$EMSCRIPTEN/system/include/libc \ -Xlinker --no-entry \ -Xlinker --allow-undefined \ -O2 -g -o test.wasm
where EMSCRIPTEN is the path to emscripten's main directory (containing emcc.py et al). This creates a test.wasm that is invalid, quickly verifiable with wabt's wasm2wat
Sep 28 2018
Sep 27 2018
Ah, yeah I saw "dschuff planned changes to this revision." in phabricator and wasn't entirely sure what that meant, but didn't let that slow me down
Generally looks ok to me
Sep 25 2018
This seems to have broken some of the emscripten tests (binaryen2.test_poppler and binaryen2.test_the_bullet) that we run on the wasm waterfall:
Sep 5 2018
Sep 4 2018
Aug 29 2018
The change feels a bit screwy to me, but I can't think of any alternative ways to factor this that aren't worse.
Aug 27 2018
Aug 20 2018
Aug 16 2018
Jul 19 2018
Jul 18 2018
Jul 17 2018
Jul 16 2018
I kinda like the use of two separate testing mode flags, to avoid needing to refactor every test to the explicit local form right away.
Jul 12 2018
Jun 21 2018
Better would be the appropriate subdirectory of test/Transforms.
In theory because the actual crash is in DeadStoreElimination I could put it there?
- Move test, remove triple
Probably just IPO then. It's still not WebAssembly specific.
Looks very reasonable and straightforward. LGTM in spirit, but I'll wait for someone who knows Clang better.
Jun 20 2018
Jun 14 2018
Overall looks ok
Apr 27 2018
Apr 24 2018
Apr 19 2018
Apr 4 2018
Mar 30 2018
Mar 27 2018
Mar 15 2018
Mar 9 2018
WIP review, read up through WebAssemblyAsmParser::ParseInstruction
Mar 2 2018
I don't think we want to add a commandline flag for this. Better to pick a name and stick with it. If that's going to be "memory" - then fine. But there's no-one asking for it to be customised.
I agree, and I think "no one is asking for this" is an important point. I think if people ask for it in the future it will be something we can do, which is why I'm biased towards leaving this as-is for now.
Mar 1 2018
Additional thoughts of mine:
Feb 23 2018
Feb 16 2018
Function-bitcast stuff looks good to me
Feb 12 2018
- Gate MCSymbolWasm on BinFormatWasm
Oct 31 2017
Oct 9 2017
- Use CallSites, properly test exceptions
- Add invoke test
Oct 6 2017
Pretty much. In C, that is explicitly defined to be well-defined, and otherwise it's undefined behavior. Native targets ignore missing args and/or read junk bits from the argument registers, wasm traps at runtime, or with this pass we synthesize/ignore args.
Oct 4 2017
Sep 15 2017
Looks good to me (take that with a grain of salt, but this seems straightforward?)
Sep 12 2017
Aug 31 2017
As an overall comment, this is awesome. Big fan of reducing syntactic noise.
Aug 30 2017
Nits aside lgtm
Aug 24 2017
- Add basic blocks to call_constexpr test
- Test more constexpr instructions, make test name more explicit
Aug 23 2017
Aug 22 2017
- Undo refactor, just change name
If you have more stop-after options it'd probably be simpler to leave it as one field. I'll just rename it to "stop-diff-after" to avoid the name collision.
Aug 21 2017
Jul 25 2017
I'm not sure off the top of my head if this works the same on Windows' cmd.exe. If someone can check this on a Windows box that would be useful.
Jun 21 2017
@sunfish , ping?
Lots of nice changes
Jun 16 2017
The comment // An unscaled add of a register. Set it as the new base. confused me then, especially because immediately following we are dealing with Adds.
Thinking about it more deeply, this is is basically when we have getelementptr i8, i8* %pointer, i32 %someExpression, and this lowers that to the wasm equivalent of (i32.add %pointer %someExpression)?
Remove unintended files
- Guard against Addr base being set in Alloca case
- Add fast-isel-abort=1 to test
Jun 13 2017
- Re-add check before final setReg in computeAddress
- Remove unneeded continue
That's probably worth doing, the problem is that the line that does this problematic folding already checks Addr.getReg() == 0, so it doesn't actually fix the problem.
The real underlying cause is that we think we're folding Add instructions, but we haven't actually checked that the Use is an Add yet. Moving the unscaled add after the check makes that a correct assumption, and everything seems happy about it.
Note that adding the return false made the lit tests pass, and only failed on the wasm waterfall. I added a test case (@store_i8_with_array_alloca_gep) to cover it.
This also only fails with the -elf triple, because we store the stack pointer in memory instead of in a global, making allocas loads instead of get_globals.
- Remove commented out code
- Reduce nesting in computeAddress, add test case for GEP flattening
Jun 8 2017
Looks fine but I don't understand it really. @sunfish ?
Jun 6 2017
Jun 5 2017
Overall looks reasonable to me? Take that for what it's worth.