Closed Bug 1141407 Opened 6 years ago Closed 11 months ago

Ion/OdinMonkey: Optimize sign-extended comparisons

Categories

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

x86_64
All
enhancement

Tracking

()

RESOLVED WONTFIX

People

(Reporter: sunfish, Unassigned)

References

(Blocks 2 open bugs)

Details

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.
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.
Blocks: jsperf
Priority: -- → P5
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.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Component: JavaScript Engine: JIT → Javascript: WebAssembly

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.

Status: REOPENED → RESOLVED
Closed: 2 years ago11 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.