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

[Jacob Gravelle via Phabricator via llvm-commits]

jgravelle-google added a comment.

In https://reviews.llvm.org/D34044#782696, @sunfish wrote:

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


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)`?

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

Ah, makes sense. Added asserts and an `isSet` function that should guard against these sorts of thing.


https://reviews.llvm.org/D34044