Closed Bug 863867 Opened 7 years ago Closed 7 years ago

OdinMonkey: Incorrect behavior on Freetype without relooping

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- fixed
firefox23 --- fixed

People

(Reporter: azakai, Unassigned)

Details

Attachments

(3 files, 3 obsolete files)

Attached file norelooped freetype (obsolete) —
Attached is freetype compiled to asm.js, without relooping (so switches in loops for each function). With odinmonkey it behaves incorrectly, whereas with --no-asmjs it produces the same correct results as node.

This codebase does work properly with relooping, so might be related to the switch construct somehow.
Attached file failure (zipped) (obsolete) —
Attachment #739792 - Attachment is obsolete: true
Attached file failure (zipped) (obsolete) —
Attachment #739877 - Attachment is obsolete: true
I am running lithium on this now, with mjrosenberg's help.
Marty: I've tracked this down to what really seems like a bug in general Ion GVN.  If you gunzip the attachment, you can find these statements:

    asmPrintInt($p_offset_0151|0);
    $148 = ($p_offset_0151 + 1) | 0;  // FAIL
    asmPrintInt($148|0);

When you run the .js file, you can see that the second printed int is not 1 plus the first printed int.  (After this, we wind up on some path we're never supposed to be on and throw.)

In the debugger, I can see that the MAdd produced for this statement is getting replaced by the add for the statement:

    $124 = $p_start_0148 + 1 | 0;

a couple statements above.  I can break in gdb and watch this happen.  Looking at how p_start_0148 is defined, it's clear these two shouldn't be GVNed.

I'd debug this further, but I don't really understand GVN, could you look at this?  It's easy to watch this under a debugger.  Just use the condition:

  (gdb) b CheckAddOrSub if f.m_.tokenStream_.srcCoords.lineNum(expr->pn_pos.begin) == 55411

to break on the creation of the MAdd node on the // FAIL line and then break in ValueNumbering before the lookupValue() to see this MAdd get unified with the other MAdd.
Is it still worth continuing to run lithium on this?
If it is just running in the background, sure, but if it is impeding you in any way, then feel free to kill it off
Attachment #739882 - Attachment is obsolete: true
the most annoying to track down bugs usually have the simplest patches.
Attachment #740769 - Flags: review?(luke)
I am still running lithium on this, I guess I can stop now? ;) It takes forever on this testcase, at one point it filled up my hard drive with those temp files and aborted due to lack of disk space. After that I disabled saving the "boring" temp files, but it still takes multiple hours to do a single execution, and running it again makes it start reducing once more. It's down from 4.5MB to 1MB so far.
sure.  could you possibly post the partially reduced testcase? I suspect we'll want this to go into jit-tests.  also, did you run with --no-asmjs or --ion-gvn=off?
Nice job tracking that down!  It'd be really good to get a testcase for this.  It seems like you could put an

  assert(number != rep->valueNumber || [[value-number-is-up-to-date]]);

right before the fix (which should assert for the giant testcase) and use this assert to derive small testcase.  You should be able to start by first removing all other functions except the one that hits the bug and removing all calls inside that one (since I'd expect that calls don't contribute to the GVN bug).  From there, reduction should be fast and easy enough to do manually.
Attached file much-reduced
This is a much-reduced testcase with --no-asmjs. It can still be reduced but it is around 4 hours for a few dozen lines at this point.

This took about 4 days, went from 4.7 MB to 0.5 MB.
Comment on attachment 740769 [details] [diff] [review]
/home/mjrosenb/patches/prematurelyOptimized-r0.patch

Thanks Alon!  Marty: can you include the reduced testcase?  Flipping the review over to Sean.
Attachment #740769 - Flags: review?(luke) → review?(sstangl)
(Note: to actually make this a good jit-test, we need it to fail in the harness (so, throw an exception), not just produce different output.)
Here's a reduced testcase:

assertEq((function() {
  'use asm';
  function _main() {
    var $1=0, $2=0, $3=0, $4=0, $5=0, $6=0, $7=0, $8=0, $9=0, $10=0, label=0;
    label = 1;
    while (1) {
      switch (label | 0) {
       case 1:
        $2 = $1 + 14 | 0;
        $3 = $1;
        label = 20;
        break;
       case 20:
        $5 = $2;
        $4 = $3;
        label = 24;
        break;
       case 24:
        $7 = $5 + 1 | 0;
        $8 = $4 + 1 | 0;
        return $8|0;
       case 49:
        $9 = $6 + 1 | 0;
        if ($10) {
          $6 = $9;
          break;
        }
        return 0;
      }
    }
    return 0;
  }
  return _main;
})()(), 1);
Attachment #740769 - Flags: review?(sstangl) → review+
Comment on attachment 740769 [details] [diff] [review]
/home/mjrosenb/patches/prematurelyOptimized-r0.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Ion GVN
User impact if declined: incorrect execution results
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): quite low
Attachment #740769 - Flags: approval-mozilla-aurora?
Attachment #740769 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/fab7a1635246
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 995542
No longer depends on: 995542
You need to log in before you can comment on or make changes to this bug.