Bug 145295 - DFG::PutStackSinkingPhase should not treat the stack variables written by LoadVarargs/ForwardVarargs as being live
Summary: DFG::PutStackSinkingPhase should not treat the stack variables written by Loa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks: 149409
  Show dependency treegraph
 
Reported: 2015-05-21 22:26 PDT by Filip Pizlo
Modified: 2015-11-09 16:39 PST (History)
2 users (show)

See Also:


Attachments
patch (11.00 KB, patch)
2015-11-09 12:54 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (12.69 KB, patch)
2015-11-09 14:39 PST, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-05-21 22:26:46 PDT
...
Comment 1 Filip Pizlo 2015-11-05 10:17:59 PST
It would be great if PutStackSinkingPhase didn't have to be precise about this.

What's your argument for why the weird IR that PutStackSinkingPhase generates is wrong?  Could we just formalize the concept that a stray GetStack can load garbage so long as the value is irrelevant?
Comment 2 Saam Barati 2015-11-06 12:19:03 PST
(In reply to comment #1)
> It would be great if PutStackSinkingPhase didn't have to be precise about
> this.
> 
> What's your argument for why the weird IR that PutStackSinkingPhase
> generates is wrong?  Could we just formalize the concept that a stray
> GetStack can load garbage so long as the value is irrelevant?

I don't have an argument yet. I really need to investigate it more, I currently don't really know why we're invalidating the block we crash on as unreachable when it really is reachable. My hypothesis is we're generating dead code that the DFG thinks isn't dead b/c of how it will sink PutStacks to LoadVarargs even when the LoadVarargs is about to write to the same stack location as the PutStack. This is probably causing something up the chain to invalidate the block we're crashing in.

I'll post more as I find out more.
Comment 3 Filip Pizlo 2015-11-06 12:40:39 PST
(In reply to comment #2)
> (In reply to comment #1)
> > It would be great if PutStackSinkingPhase didn't have to be precise about
> > this.
> > 
> > What's your argument for why the weird IR that PutStackSinkingPhase
> > generates is wrong?  Could we just formalize the concept that a stray
> > GetStack can load garbage so long as the value is irrelevant?
> 
> I don't have an argument yet. I really need to investigate it more, I
> currently don't really know why we're invalidating the block we crash on as
> unreachable when it really is reachable. My hypothesis is we're generating
> dead code 

Let's not use the term "dead code".  I propose the following terms:

- Unreachable code.  This is code that cannot be reached by any control flow path.
- Always-exiting code.  This is a special kind of unreachable code, where you are unreachable because anyone who could have a path to you will always exit.
- Code with unused results.  This is code that has no side effects and produces a value that nobody consumes.  This is traditionally called "dead code".

I'm pretty sure you don't mean that the code is dead in the sense that its results are unused.  A PutStack is an effectful operation so it is never dead.  Same for LoadVarargs.  Probably, you are talking about unreachable code.

> that the DFG thinks isn't dead b/c of how it will sink PutStacks
> to LoadVarargs even when the LoadVarargs is about to write to the same stack
> location as the PutStack. 

This does not seem plausible.  The fact that you did a PutStack that gets overwritten has no effect on whether code is reachable or alive.

> This is probably causing something up the chain to
> invalidate the block we're crashing in.
> 
> I'll post more as I find out more.
Comment 4 Saam Barati 2015-11-09 12:54:45 PST
Created attachment 265084 [details]
patch
Comment 5 WebKit Commit Bot 2015-11-09 12:57:42 PST
Attachment 265084 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:119:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:278:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:491:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Filip Pizlo 2015-11-09 13:24:08 PST
Comment on attachment 265084 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265084&action=review

You should also probably remove or edit this comment:

        // This code has some interesting quirks because of the fact that neither liveness nor
        // deferrals are very precise. They are only precise enough to be able to correctly tell us
        // when we may [sic] need to execute PutStacks. This means that they may report the need to
        // execute a PutStack in cases where we actually don't really need it, and that's totally OK.

> Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:122
> +
> +                    auto writeHandler = [&] (VirtualRegister operand) {
> +                        RELEASE_ASSERT(node->op() == PutStack || node->op() == LoadVarargs || node->op() == ForwardVarargs);
> +                        live.operand(operand) = false;
> +                    };

This seems kind of dangerous.  What if an operation both reads and writes a local?  In that case, it's live.  But you'll make it dead.
Comment 7 Geoffrey Garen 2015-11-09 13:56:02 PST
Comment on attachment 265084 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265084&action=review

> Source/JavaScriptCore/ChangeLog:21
> +        block would indeed be reached.The solution here is to be more precise about the 

. The

> Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:110
> -                    
> +

Revert.
Comment 8 Saam Barati 2015-11-09 14:39:02 PST
Created attachment 265099 [details]
patch

Enforced correct liveness by executing writes before reads.
I also edited the comment to describe that being imprecise
about liveness can be dangerous because it can cause us
to insert GetStacks in regions where such a GetStack
makes no sense and would load garbage.
Comment 9 WebKit Commit Bot 2015-11-09 14:41:37 PST
Attachment 265099 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:288:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:501:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Filip Pizlo 2015-11-09 16:11:35 PST
Comment on attachment 265099 [details]
patch

Nice!
Comment 11 Saam Barati 2015-11-09 16:39:40 PST
landed in:
http://trac.webkit.org/changeset/192190