1276009 - IonMonkey should fold x^-1 to ~x.

Status: ASSIGNED

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

Description

[Dan Gohman [:sunfish]]

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

Comment 1

[Dan Gohman [:sunfish]]

Attachment: bitxor-neg-one.patch

WebAssembly doesn't have a ~ operator, so it's useful to pattern-match x^-1 so that we can turn it into MBitNot.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 3

[Jan de Mooij [:jandem]]

Dan, can we finish/land this? Comment 1 suggests it could be a nice little memory/perf win on wasm code.