Open Bug 1276009 Opened 9 years ago Updated 1 year ago

IonMonkey should fold x^-1 to ~x.

Categories

(Core :: JavaScript Engine: JIT, defect, P3)

defect

Tracking

()

ASSIGNED

People

(Reporter: sunfish, Assigned: jseward)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(1 file)

C/C++: =============================== int x(int y) { return ~y; } Firefox: =============================== wasm-function[0]: sub rsp, 0x18 ; 0x000020 48 83 ec 18 mov eax, edi ; 0x000024 8b c7 xor eax, 0xffffffff ; 0x000026 83 f0 ff nop ; 0x000029 66 90 add rsp, 0x18 ; 0x00002b 48 83 c4 18 ret ; 0x00002f c3 LLVM: =============================== .text .intel_syntax noprefix .file "upload/21f07f3395b5ef50cd16db563a54ded3.cpp" .globl _Z1xi .type _Z1xi,@function _Z1xi: .cfi_startproc not edi mov eax, edi ret .Lfunc_end0: .size _Z1xi, .Lfunc_end0-_Z1xi .cfi_endproc .ident "clang version 3.9.0 (trunk 270097)" .section ".note.GNU-stack","",@progbits
Attached patch bitxor-neg-one.patch (deleted) — Splinter Review
WebAssembly doesn't have a ~ operator, so it's useful to pattern-match x^-1 so that we can turn it into MBitNot.
Assignee: nobody → sunfish
Attachment #8757069 - Flags: review?(bbouvier)
Comment on attachment 8757069 [details] [diff] [review] bitxor-neg-one.patch Review of attachment 8757069 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, but it seems there is already a framework to do so: you could just implement MBitXor::foldIfNegOne() instead. Also, the comment in https://dxr.mozilla.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#3180-3182 makes me think that your patch would prevent optimizations. Can you try this instead, please? ::: js/src/jit/MIR.cpp @@ +2484,5 @@ > } > > MDefinition* > +MBitXor::foldsTo(TempAllocator& alloc) > +{ MBitXor inherits from MBinaryBitwiseInstruction, which also has a foldsTo method. Can you call the parent method at the top first? @@ +2487,5 @@ > +MBitXor::foldsTo(TempAllocator& alloc) > +{ > + MDefinition* lhs = getOperand(0); > + MDefinition* rhs = getOperand(1); > + MBitNot *bitNot = nullptr; nit: MBitNot* bitnot @@ +2498,5 @@ > + if (rhs->isConstant()) { > + MConstant *rhsConst = rhs->toConstant(); > + if (rhsConst->isInt32(-1) || rhsConst->isInt64(-1)) > + bitNot = MBitNot::New(alloc, lhs); > + } Can probably do something more compact by testing if (lhs->isConstant() || rhs->isConstant()) and then swap operands so that the constant is in e.g. lhs, as bitxor is commutative.
Attachment #8757069 - Flags: review?(bbouvier)
Dan, can we finish/land this? Comment 1 suggests it could be a nice little memory/perf win on wasm code.
Blocks: jsperf
Flags: needinfo?(sunfish)
Priority: -- → P3
Keywords: perf
Assignee: sunfish → jseward
Status: NEW → ASSIGNED
Severity: normal → S3
Flags: needinfo?(sunfish)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: