1277086 - IonMonkey should fold 0-x to -x.

Status: RESOLVED DUPLICATE of bug 1710403

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

Description

[Yury Delendik (:yury)]

WasmExplorer: http://goo.gl/jqCYdd
C/C++:
===============================
int t(int i) { return -i; }

Firefox:
===============================
wasm-function[0]:
  sub rsp, 8                            ; 0x000020 48 83 ec 08
  xor eax, eax                          ; 0x000024 33 c0
  sub eax, edi                          ; 0x000026 2b c7
  nop                                   ; 0x000028 66 90
  add rsp, 8                            ; 0x00002a 48 83 c4 08
  ret                                   ; 0x00002e c3



LLVM:
===============================
	.type	_Z1ti,@function
_Z1ti:
	.cfi_startproc
	neg	edi
	mov	eax, edi
	ret

Comment 1

[Jan de Mooij [:jandem]]

Wasm code probably uses this quite a lot so we should just emit good code for this.

I think Ion compiles -x as x * -1, and codegen for the multiplication then folds this to a neg instruction, but we can't really optimize 0 - x like that. One option is to have GVN fold 0 - int32 to int32 * -1, or we should add an MNegate op..

Comment 2

[Hannes Verschore [:h4writer]]

This code for this is in js/src/jit/MIR.cpp and the function doing this is "foldsTo".
The idea is that MSub with "0 - int32" can get folded to "int32 * -1".

Comment 3

[Taahir Ahmed]

Attachment: 0001-Bug-1277086-Fold-0-x-to-x-1.patch

I think the logic here is correct, but I'm having trouble triggering the optimization in the test suite.  Even if I do something absurd like fold `0-x` to `x*-10`, the modified test still passes.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8811544 [details] [diff] [review]
0001-Bug-1277086-Fold-0-x-to-x-1.patch

Review of attachment 8811544 [details] [diff] [review]:
-----------------------------------------------------------------

You might want to test what is going wrong. If you create a small testcase you can see what IonMonkey is doing and that might give an explanation why it is not working.

E.g.
function test(x) {
    return 0 - x;
}
test(4)
test(3)

Now if you run with "--ion-eager --no-threads" we will immediately go try to execute this in IonMonkey.
$ $JS/dist/bin/js --ion-eager --no-threads test.js

You can see if that compiles things by adding the environment flag: IONFLAGS=scripts 
[IonScripts] Compiling script XXX (0xXXXXXXXXXX) (warmup-counter=X, level=Optimization_XXX)
Note: This needs a debug build! Make sure you have a debug build (--enable-debug)

That means we are compiling that function.

Now to get more information you might want to get iongraph:
https://github.com/sstangl/iongraph

If you have that run with the environment flag: IONFLAGS=logs
That will create files in /tmp/ (if you are on linux, else it will be in the current directory)
The file ion.json is the most important here.

Run it through iongraph and you will get a lot of files:
func%s-pass%s-%s-%s.gv

(you can read gv files with xdot or create pdf/png out of it. There are help commands in the repo.

You can look at those files and see what happens in the transformation.
You will probably have to look to the last compiled function.
I would suggest looking at pass0 to verify the MSub is present in that graph.
Afterwards look at the "GVN" pass which is after GVN has happened and see if we did the optimization or not.


Had a quick look through the patch, which looks good.
Though the next step would be to confirm this optimization triggers and why it doesn't.
If you get it to trigger, please request review again.

Comment 5

[Taahir Ahmed]

Attachment: 0001-Bug-1277086-Fold-0-x-to-x-1.patch

Wow! Thanks for the detailed instructions.

Using `iongraph`, I can see that the optimization is applied if I explicitly coerce to int32:

function test(x) {
    x = x|0;
    return 0 - x;
}

The problem with the WASM test was that it wasn't testing the case `const(0) - x`.  I have adjusted that test so that it runs all variants of const and non-const operands.  I can see the optimization being applied in WASM mode as well.

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8812321 [details] [diff] [review]
0001-Bug-1277086-Fold-0-x-to-x-1.patch

Review of attachment 8812321 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work! Was wondering if the "0 - X" to "-X" works in float/double cases?

::: js/src/jit/MIR.cpp
@@ +3167,1 @@
>          return this;

It is not needed to add "isSub()" here.
The first check if rhs is the identity (=0) is allowed on floats.

-0 + 0 = -0

@@ +3180,5 @@
> +        if (isTruncated()) {
> +            block()->insertBefore(this, negated);
> +            return MTruncateToInt32::New(alloc, negated);
> +        }
> +        return negated;

Could you add tests if this works on floats (NaN, +/-Infinity, large nummbers ...) and add test for that. And if does not work for those you can add the specialization_ check on in here.

@@ +3186,2 @@
>  
>      if (IsConstant(lhs, getIdentity())) {

Can you add MOZ_ASSERT(!isSub()); here?

Comment 7

[Jeff Walden [:Waldo]]

(In reply to Hannes Verschore [:h4writer] from comment #6)
> Was wondering if the "0 - X" to "-X" works in float/double cases?

If X is +0, then (0 - +0) is (0 + -0) is +0, which is not -X.

> -0 + 0 = -0

Uh...that's not right.  The sum of two zeroes is -0 iff both zeroes are -0.  -0 + 0 is supposed to be +0.

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> (In reply to Hannes Verschore [:h4writer] from comment #6)
> > Was wondering if the "0 - X" to "-X" works in float/double cases?
> 
> If X is +0, then (0 - +0) is (0 + -0) is +0, which is not -X.
> 

Right. Good observation. We should only allow Int32 for the 0 - X to -X transformation.

> > -0 + 0 = -0
> 
> Uh...that's not right.  The sum of two zeroes is -0 iff both zeroes are -0. 
> -0 + 0 is supposed to be +0.

Oh indeed. Getting too late apparently. That should have been subtraction: -0 - 0 = -0.

Comment 9

[Hannes Verschore [:h4writer]]

Hey Ahmed, how is it going? Any questions on the given review?

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sdetar, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 11

[Lars T Hansen [:lth]]

We want this for Wasm and it blocks the appropriate bug.

Comment 12

[Abhishek Sharma]

Attachment: Bug 1277086: Fold 0-x to -1*x for Int32 during GVN. r=jandem

Comment 13

[Abhishek Sharma]

Hi,
Decided to have a go at this. I did the smallest possible change I could understand and seemed to work from some initial testing. I am unsure about -

1. What sort of testing/tests I need to add and where.
2. How truncation works for MIR instructions. Some of the other folds seem to check if the isTruncate returns true for the original instruction and if so pass the output of the folded operation to MTruncateToInt32. I'm not sure if I need to do that for this optimization.
3. Since the earlier conversation on this thread mentioned only Int32 I have handled the Int32 case for now. But I feel like this optimization should be applicable for the Int64 case too ?

Comment 14

[Jan de Mooij [:jandem]]

Thanks a lot for the patch!

I see Lars already fixed this for the Wasm case in comment 0 in the LIR backend in bug 1710403. For JS, I think we shouldn't fold 0 - x to -1 * x because there's a subtle difference in behavior: 0 - 0 ==> +0, but -1 * 0 ==> -0. What this means it that if we fold 0 - x to -1 * x, we would now trigger a bailout (deoptimization) if x == 0 (because the result, -0, doesn't fit in an int32) where we didn't have a bailout before.

For cases where we don't care about -0, for example (0 - x)|0, the optimization added in bug 1710403 already kicks in and we generate a negl instruction.

Given all this, I don't think there's anything left to do here unfortunately :/

Comment 16

[Abhishek Sharma]

Makes sense. Thanks for the explanation/clarification!