This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] WebAssemblyFastISel getelementptr variable index support
ClosedPublic

Authored by jgravelle-google on Jun 8 2017, 12:00 PM.

Details

Event Timeline

sunfish edited edge metadata.Jun 9 2017, 4:42 PM

The underlying problem is that after it folds the dynamic index into the address base register, the code at the bottom of WebAssemblyFastISel::computeAddress just sticks the getelementptr base into the address base register, accidentally overwriting the previous value. 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.

For extra caution, it may be possible to have Address::setReg assert that the base is zero before overwriting it.

  • Reduce nesting in computeAddress, add test case for GEP flattening
  • Remove commented out code
sbc100 added inline comments.Jun 13 2017, 2:36 PM
lib/Target/WebAssembly/WebAssemblyFastISel.cpp
297

Is this continue now redundant?

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.

For extra caution, it may be possible to have Address::setReg assert that the base is zero before overwriting it.

Probably a good idea going forward. Added.

@sunfish PTAL

  • Remove unneeded continue
jgravelle-google marked an inline comment as done.Jun 13 2017, 2:59 PM
jgravelle-google added inline comments.
lib/Target/WebAssembly/WebAssemblyFastISel.cpp
297

Yep, thanks

jgravelle-google marked an inline comment as done.Jun 13 2017, 3:06 PM
jgravelle-google removed subscribers: jholewinski, axw, MatzeB and 7 others.
  • Re-add check before final setReg in computeAddress
dschuff added inline comments.Jun 15 2017, 5:09 PM
test/CodeGen/WebAssembly/offset-fastisel.ll
2

Add -fast-isel-abort=1

  • Add fast-isel-abort=1 to test
jgravelle-google marked an inline comment as done.Jun 16 2017, 10:56 AM

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.

  • Guard against Addr base being set in Alloca case

Remove unintended files

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.

sunfish accepted this revision.Jun 22 2017, 1:31 PM

Looks good!

This revision is now accepted and ready to land.Jun 22 2017, 1:31 PM
This revision was automatically updated to reflect the committed changes.