emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.8k stars 3.31k forks source link

Invalid ASM.JS code is generated due to math operation. #1685

Closed onnoj closed 11 years ago

onnoj commented 11 years ago

Hi,

The following code (from CEGUI):

//From
const float ColourPickerControls::LAB_L_MIN(0.0f);
const float ColourPickerControls::LAB_L_MAX(100.0f);
const float ColourPickerControls::LAB_L_DIFF(LAB_L_MAX - LAB_L_MIN);
const float ColourPickerControls::LAB_A_MIN(-130.0f);
const float ColourPickerControls::LAB_A_MAX(130.0f);
const float ColourPickerControls::LAB_A_DIFF(LAB_A_MAX - LAB_A_MIN);
const float ColourPickerControls::LAB_B_MIN(-130.0f);
const float ColourPickerControls::LAB_B_MAX(130.0f);
const float ColourPickerControls::LAB_B_DIFF(LAB_B_MAX - LAB_B_MIN);

//..snipped for brevity..

Vector2f ColourPickerControls::getColourPickingColourPosition()
{
    float x;
    float y;

    switch (d_sliderMode)
    {
    case SliderMode_Lab_L:
        x = 1.0f - (d_selectedColourLAB.a - LAB_A_MIN) / LAB_A_DIFF;
        y = 1.0f - (d_selectedColourLAB.b - LAB_B_MIN) / LAB_B_DIFF;
        break;

    case SliderMode_Lab_A:
        x = 1.0f - (d_selectedColourLAB.b - LAB_B_MIN) / LAB_B_DIFF;
        y = 1.0f - (d_selectedColourLAB.L - LAB_L_MIN) / LAB_L_DIFF;
        break;

    case SliderMode_Lab_B:
        x = 1.0f - (d_selectedColourLAB.a - LAB_A_MIN) / LAB_A_DIFF;
        y = 1.0f - (d_selectedColourLAB.L - LAB_L_MIN) / LAB_L_DIFF;
        break;

    case SliderMode_HSV_H:
        x = d_selectedColourHSV.S;
        y = 1.0f - d_selectedColourHSV.V;
        break;

    case SliderMode_HSV_S:
        getColourPickingColourPositionHSV(x, y);
        break;

    case SliderMode_HSV_V:
        getColourPickingColourPositionHSV(x, y);
        break;

    default:
        break;
    }

    return Vector2f(x, y);
}

Is compiled to the following javascript:

function dDJ(a,b){a=a|0;
  b=b|0;
  var d=0,e=0,f=0,h=0;
  d=i;
  i=i+16|0;
  e=d|0;
  f=d+8|0;
  h=b;
  switch(c[h+1448>>2]|0){case 16:{dDK(h,e,f);
      break};
  case 2:{g[e>>2]=1.0-(+g[h+1488>>2]--130.0)/260.0;
      g[f>>2]=1.0-(+g[h+1480>>2]-0.0)/100.0;
      break};
  case 4:{g[e>>2]=1.0-(+g[h+1484>>2]--130.0)/260.0;
      g[f>>2]=1.0-(+g[h+1480>>2]-0.0)/100.0;
      break};
  case 1:{g[e>>2]=1.0-(+g[h+1484>>2]--130.0)/260.0;
      g[f>>2]=1.0-(+g[h+1488>>2]--130.0)/260.0;
      break};
  case 32:{dDK(h,e,f);
      break};
  case 8:{g[e>>2]=+g[h+1496>>2];
      g[f>>2]=1.0- +g[h+1500>>2];
      break};
  default:{}}aQc(a,+g[e>>2],+g[f>>2]);
  i=d;
  return}

Resulting in error: case 2:{g[e>>2]=1.0-(+g[h+1488>>2]--130.0)/260.0; Uncaught SyntaxError: Unexpected number

I have attached uploaded a sample. Please note that while the sample compiles & runs, it does not execute succesfully. Furthermore, I had to compile it with LINKABLE=1 because otherwise the code would be generated differently (or not at all?). The actual project is compiled with: -s FULL_ES2=1 -s VERBOSE=1 \ -s ASM_JS=1 -s FORCE_GL_EMULATION=1 \ -s TOTAL_MEMORY=$(expr 128 \* 1024 \* 1024) \ -O2 --js-library ../library_EERT.js \ -s TOTAL_STACK=10000000 \

It would seem that it's a similar bug as described in Issue: #501 Code was compiled with Clang/LLVM 3.2 stable, node v0.10.18 and a yesterday checkout of Emscripten's main branch.

kripken commented 11 years ago

Can you provide a full testcase (including necessary headers etc.)? If that's what's in the 7z file, can you attach a zip? Ubuntu doesn't have 7-z support by default it seems.

onnoj commented 11 years ago

Here you go: https://dl.dropboxusercontent.com/u/18328980/asmCodeGenIssue.zip

onnoj commented 11 years ago

There is a build.sh that invokes the commands that are effectively invoked by my own build environment. libscombined.bc was produced by llvm-link merging the various libs. The CEGUI lib was compiled using CMake & Emscripten_unix.cmake.

onnoj commented 11 years ago

And I recompiled CEGUI using the new emscripten.cmake file, just in case some compilation flags had changed, but still the same issue.

ghost commented 11 years ago

I'm seeing a similar problem, where the generated ASM.js code includes the following:

function __ZN2cvL16interpolateCubicEfPf($x,$coeffs) {
  $x=+$x;
  $coeffs=$coeffs|0;
  var $1=0.0,$2=0;$1=$x;$2=$coeffs;
  HEAPF32[$2>>2]=((($1+1.0)*-.75+3.75)*($1+1.0)+-6.0)*($1+1.0)--3.0;
  HEAPF32[$2+4>>2]=($1*1.25-2.25)*$1*$1+1.0;
  HEAPF32[$2+8>>2]=((1.0-$1)*1.25-2.25)*(1.0-$1)*(1.0-$1)+1.0;
  HEAPF32[$2+12>>2]=1.0- +HEAPF32[$2>>2]- +HEAPF32[$2+4>>2]- +HEAPF32[$2+8>>2];
  return
}

compiled from

static inline void interpolateCubic( float x, float* coeffs )
{
    const float A = -0.75f;

    coeffs[0] = ((A*(x + 1) - 5*A)*(x + 1) + 8*A)*(x + 1) - 4*A;
    coeffs[1] = ((A + 2)*x - (A + 3))*x*x + 1;
    coeffs[2] = ((A + 2)*(1 - x) - (A + 3))*(1 - x)*(1 - x) + 1;
    coeffs[3] = 1.f - coeffs[0] - coeffs[1] - coeffs[2];
}

This is part of OpenCV. I've built OpenCV to static libraries (in LLVM bitcode), which I think by default uses -O3 and am then compiling it with other code that uses the library with -O1.

I've tried just compiling this function by itself with -O1 but get different generated code.

kripken commented 11 years ago

Sorry for the delay here, I was at a conference. Thanks for the zip, looks like it should be easy to debug.

onnoj commented 11 years ago

Thanks, it is appreciated! Let me know if I can do anything.

kripken commented 11 years ago

Ok, this should be fixed on incoming now. It's a minifier bug, but we've seen it on 2 minifiers now, so I worked around it in the js optimizer (and it's better to emit X+Y instead of (X- -Y) anyhow).

ghost commented 11 years ago

Thanks - this fixes the problem I was seeing.

onnoj commented 11 years ago

Thanks!