dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.3k stars 1.59k forks source link

Possible optimization: checking ioore exception once inside of loop #15679

Open sethladd opened 10 years ago

sethladd commented 10 years ago

Consider this code:

  for (int i=0; i<lights.length; i+=4) {     for (int j=0; j<4; j++) {       if ((i+j)<lights.length) {         var l = lights[i+j];         posData[j4] = l.pos.x;         posData[j4] = l.pos.y;         posData[j4] = l.pos.z;         posData[j4] = l.size;         colorData[j3+0] = l.color.x;         colorData[j3+1] = l.color.y;         colorData[j*3+2] = l.color.z;

dart2js generates this:

    for (lights.length, i = 0; i < 2; i += 4)       for (j = 0; j < 4; ++j) {         t1 = i + j;         t2 = j 3;         t3 = j 4;         if (t1 < 2) {           l = lights[t1];           t1 = $.get$posData();           t4 = l.pos;           t5 = t1.length;           if (t3 >= t5)             return H.ioore(t1, t3);           t1[t3] = t4.x;           if (t3 >= t5)             return H.ioore(t1, t3);           t1[t3] = t4.y;           if (t3 >= t5)             return H.ioore(t1, t3);           t1[t3] = t4.z;           if (t3 >= t5)             return H.ioore(t1, t3);           t1[t3] = l.size;

Notice how t3 >= t5 is checked many times. Do we have sufficient information to determine that t3 won't change in the loop, and we only need to check t3 >= t5 only once?

sethladd commented 10 years ago

(note: the real code wouldn't do posData[j4] over and over, it would do posData[j4+0,1, etc) I'm just experimenting

DartBot commented 10 years ago

This comment was originally written by ngeoffray@google.com


Hi Seth,

Good catch :-) You ran into an optimization ordering issue, where code optimization B gives more room for code optimization A that unfortunately ran before B.

@floitsch, kevin: we can either re-run GVN, or make the HLazyStatic instruction GVN'able. Blame Florian for making it non-GVN'able in the first place :-)


cc @kmillikin. cc @floitschG.

floitschG commented 10 years ago

Expanding on Nicolas' explanation: you can get better code, if you assign posData to a local variable. The issue is, that it is a lazily initialized static, and that accessing it might therefore run through code that has side-effect.

See issue #4931.

sethladd commented 10 years ago

Adding this to the Dart function:

var posData = posData2; // bring the variable inside the loop

Produces this code:

  render: function(lights) {     var posData, t1, i, j, t2, t3, t4, l, t5, t6;     posData = $.get$posData2();     for (lights.length, t1 = posData.length, i = 0; i < 2; i += 4)       for (j = 0; j < 4; ++j) {         t2 = i + j;         t3 = j 4;         t4 = j 3;         if (t2 < 2) {           l = lights[t2];           t2 = l.pos;           if (t3 >= t1)             return H.ioore(posData, t3);           posData[t3] = t2.x;           posData[t3] = t2.y;           posData[t3] = t2.z;           posData[t3] = l.size;           t3 = $.get$colorData();           t2 = t4 + 0;           t5 = l.color;           t6 = t3.length;           if (t2 >= t6)             return H.ioore(t3, t2);           t3[t2] = t5.x;           t2 = t4 + 1;           if (t2 >= t6)             return H.ioore(t3, t2);           t3[t2] = t5.y;           t4 += 2;           if (t4 >= t6)             return H.ioore(t3, t4);           t3[t4] = t5.z;

sethladd commented 10 years ago

Added C6 label.

floitschG commented 10 years ago

Just to be sure: the code in comment #­4 is what we want to have. right?

sethladd commented 10 years ago

This is what I was happy to see:

          posData[t3] = t2.x;           posData[t3] = t2.y;           posData[t3] = t2.z;           posData[t3] = l.size;

sethladd commented 10 years ago

Now, the real loop does this:

          posData[j4+0] = l.pos.x;           posData[j4+1] = l.pos.y;           posData[j4+2] = l.pos.z;           posData[j4+3] = l.size;

Which generates JS like this (lots of ioore checks):

            t5 = t8 + 0;             t6 = l.pos.storage;             t9 = t6[0];             if (t5 < 0 || t5 >= t3)               return H.ioore(pd, t5);             pd[t5] = t9;             t9 = t8 + 1;             t5 = t6[1];             if (t9 < 0 || t9 >= t3)               return H.ioore(pd, t9);             pd[t9] = t5; ... and so on...

Is there a way I can convince dart2js that j4+3 is inside the index bounds for my Float32List ? Currently I have a Float32List(44);

I suppose I could make a List of Float32Lists. That strategy generates this:

          t8 = j 4;           t9 = j 3;           if (t7 < lights.length) {             l = lights[t7];             if (t8 >= 4)               return H.ioore(t6, t8);             t7 = l.pos.storage;             t6[t8][0] = t7[0];             t8 = t6[t8];             t8[1] = t7[1];             t8[2] = t7[2];             t8[3] = l.size;

from this code:

          datas[j4][0] = l.pos.x;           datas[j4][1] = l.pos.y;           datas[j4][2] = l.pos.z;           datas[j4][3] = l.size;

A bit cleaner, but not what I want.

Thoughts?

sethladd commented 10 years ago

Issue #16560 has been merged into this issue.

sethladd commented 10 years ago

For another example code, see #­16560

rakudrama commented 10 years ago

Issue #16560 has been merged into this issue.

sethladd commented 10 years ago

Issue #16560 has been merged into this issue.

sethladd commented 10 years ago

FYI Stephan suggested this strategy:

https://code.google.com/p/dart/issues/detail?id=16560#c4

sethladd commented 10 years ago

Issue #16560 has been merged into this issue.


cc @rakudrama.

floitschG commented 10 years ago

Issue #16560 has been merged into this issue.

floitschG commented 10 years ago

Added Optimization label.

kasperl commented 10 years ago

Issue #16560 has been merged into this issue.

sethladd commented 10 years ago

Issue #16560 has been merged into this issue.

sethladd commented 10 years ago

Issue #16560 has been merged into this issue.