1141407 - Ion/OdinMonkey: Optimize sign-extended comparisons

Status: RESOLVED WONTFIX

(Core :: JavaScript: WebAssembly, enhancement, P5)

Description

[Dan Gohman [:sunfish]]

Consider this C code:

#include <stdio.h>

char x[2424];

int main(int argc, char *argv[])
{
    if (x[argc] == 50 || x[argc] == 51) {
        printf("hello world\n");
    }
    return 0;
}

Emscripten compiles it to this asm.js code (-O2 -profiling)

function _main(i1, i2) {
 i1 = i1 | 0;
 i2 = i2 | 0;
 i2 = STACKTOP;
 if ((HEAP8[8 + i1 >> 0] & -2) << 24 >> 24 != 50) {
  STACKTOP = i2;
  return 0;
 }
 _puts(2432) | 0;
 STACKTOP = i2;
 return 0;
}

Odin compiles it to this x86 code:

[Codegen] instruction AsmJSLoadHeap
[Codegen] movsbl     0x8(%r15,%rdi,1), %ecx
[Codegen] instruction BitOpI:bitand
[Codegen] andl       $0xfffffffe, %ecx
[Codegen] instruction ShiftI:lsh
[Codegen] shll       $24, %ecx
[Codegen] instruction ShiftI:rsh
[Codegen] sarl       $24, %ecx
[Codegen] instruction CompareAndBranch:ne
[Codegen] cmpl       $0x32, %ecx
[Codegen] je         .Lfrom19529

It'd be great if we didn't actually end up with the lsh+rsh pair here. One option is to optimize this into an instruction which uses cmpb instead of cmpl, which would avoid the need to sign-extend the input register.

This pattern shows up in a bunch of places in sqlite, for example.

Comment 1

[Douglas Crosher [:dougc]]

While not relevant to the above example (due to the bitand), it might also help to be able to perform the array element load in the comparison instruction argument rather than having a separate load. This might at least reduce register pressure. Similarly for other instructions that load from an array element or that modify an array element.

Comment 2

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 3

[Lars T Hansen [:lth]]

This is an asm.js artifact, so we won't fix it. Wasm has sign-extending load instructions that handle this well (I just checked the wasm generated by current emcc). It's possible there are other issues with sign extension, but wasm has sign-extend instructions and the producer is expected to use those.