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)
Core
JavaScript Engine: JIT
Tracking
()
ASSIGNED
People
(Reporter: sunfish, Assigned: jseward)
References
(Blocks 2 open bugs)
Details
(Keywords: perf)
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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)
Comment 3•8 years ago
|
||
Dan, can we finish/land this? Comment 1 suggests it could be a nice little memory/perf win on wasm code.
Updated•3 years ago
|
Blocks: wasm-jit-bugs
Updated•3 years ago
|
Updated•3 years ago
|
Assignee: sunfish → jseward
Status: NEW → ASSIGNED
Updated•2 years ago
|
Severity: normal → S3
Updated•1 year ago
|
Flags: needinfo?(sunfish)
You need to log in
before you can comment on or make changes to this bug.
Description
•