Ion/OdinMonkey: Optimize sign-extended comparisons

REOPENED
Unassigned

Status

()

enhancement
P5
normal
REOPENED
5 years ago
Last year

People

(Reporter: sunfish, Unassigned)

Tracking

(Blocks 2 bugs)

Firefox Tracking Flags

(Not tracked)

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: Last year
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
You need to log in before you can comment on or make changes to this bug.