Bug 134056 - [ftlopt] LICM should be able to hoist CheckStructure even if the loop clobbers structures so long as the structures being checked are watchable
Summary: [ftlopt] LICM should be able to hoist CheckStructure even if the loop clobber...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
Depends on:
Blocks: 133229
  Show dependency treegraph
Reported: 2014-06-18 22:00 PDT by Filip Pizlo
Modified: 2014-06-19 13:44 PDT (History)
7 users (show)

See Also:

the patch (10.44 KB, patch)
2014-06-18 22:24 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2014-06-18 22:00:51 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2014-06-18 22:24:10 PDT
Created attachment 233349 [details]
the patch
Comment 2 Filip Pizlo 2014-06-19 13:30:42 PDT
Comment on attachment 233349 [details]
the patch

This is totally wrong.
Comment 3 Filip Pizlo 2014-06-19 13:44:09 PDT
It's not right to hoist a CheckStructure just because the structure is watchable.  Consider a loop like:

var o = ...; // when assigned here, o does not have a field f.
for (;;) {
    if (p1)
        o.f = 42; // adds a field f
    if (p2) // p2 becomes true only after p1 had become true
        ... = o.f; // say that after adding a field f to o, we won't add any more things to o, so the CheckStructure here will be on a watchable structure set.

If we hoist the CheckStructure then the CheckStructure will be sure to fail.  Therefore, the rules on the hoistability of CheckStructures need to be substantially more subtle.  In the example above, at a minimum we need to have some profiling of the type that o has at the loop pre-header to make sure that hoisting the CheckStructure to that location won't be guaranteed to fail.