[PATCH] D34044: [WebAssembly] WebAssemblyFastISel getelementptr variable index support

[Dan Gohman via Phabricator via llvm-commits]

sunfish added a comment.

In https://reviews.llvm.org/D34044#779541, @jgravelle-google wrote:

> In https://reviews.llvm.org/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.

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

Ah, ok. Looking at that testcase, the problem is that the `case Instruction::Alloca:` code has another instance as the same bug as before; it also needs to return false if there's already a base register, before assigning a new base.


https://reviews.llvm.org/D34044