lgtm

Landed as 92ed86d239cd

Reviewed in https://reviews.llvm.org/D65916

Overall direction looks good to me as well.

• More precise help message

lgtm

Nice, lgtm

Lgtm
Seems good and simple, ship it

Forgot to add, need to add __attribute__((used, visibility("default"))) to btPersistentManifold::clearUserCache in that cpp file

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

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

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:
https://logs.chromium.org/v/?s=chromium%2Fbb%2Fclient.wasm.llvm%2Flinux%2F35933%2F%2B%2Frecipes%2Fsteps%2FExecute_emscripten_testsuite__emwasm_%2F0%2Fstdout

The change feels a bit screwy to me, but I can't think of any alternative ways to factor this that aren't worse.

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.

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.

Overall looks ok

Nice!

WIP review, read up through WebAssemblyAsmParser::ParseInstruction

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.

Additional thoughts of mine:

Function-bitcast stuff looks good to me

• Gate MCSymbolWasm on BinFormatWasm

• Use CallSites, properly test exceptions

• Add invoke test

In D38640#891197, @sunfish wrote:

Is the assumption here that the eventual caller will cast the argument back to the original type?

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.

Nice

Looks good to me (take that with a grain of salt, but this seems straightforward?)

As an overall comment, this is awesome. Big fan of reducing syntactic noise.

Nits aside lgtm

• Add basic blocks to call_constexpr test

• Test more constexpr instructions, make test name more explicit

• 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.

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.

@sunfish , ping?

Lots of nice changes

In D34044#782696, @sunfish wrote:

In D34044#779541, @jgravelle-google wrote:

In D34044#777275, @sunfish wrote:

A more precise fix would be to have that code at the bottom of WebAssemblyFastISel::computeAddress return false in the case where there's already a (non-zero) base register.

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.

The if (S == 1 && Addr.isRegBase() && Addr.getReg() == 0) case doesn't depend on the use being an Add.

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

• Re-add check before final setReg in computeAddress

• Remove unneeded continue

In D34044#777275, @sunfish wrote:

A more precise fix would be to have that code at the bottom of WebAssemblyFastISel::computeAddress return false in the case where there's already a (non-zero) base register.

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

• Rebase

• Reduce nesting in computeAddress, add test case for GEP flattening

Looks fine but I don't understand it really. @sunfish ?

Overall looks reasonable to me? Take that for what it's worth.