Bug 162342 - Optimize B3->Air lowering of Fence on ARM
Summary: Optimize B3->Air lowering of Fence on ARM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 162343
Blocks: 162316
  Show dependency treegraph
 
Reported: 2016-09-21 09:40 PDT by Filip Pizlo
Modified: 2016-09-28 13:33 PDT (History)
6 users (show)

See Also:


Attachments
the patch (14.68 KB, patch)
2016-09-28 13:01 PDT, Filip Pizlo
ggaren: 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 2016-09-21 09:40:10 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2016-09-28 13:01:41 PDT
Created attachment 290107 [details]
the patch
Comment 2 Geoffrey Garen 2016-09-28 13:20:54 PDT
Comment on attachment 290107 [details]
the patch

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

r=me

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2053
> +            if (!fence->write && !fence->read)
>                  return;

Does this happen in practice? Seems strange to specify a no-op fence. Maybe we can prohibit it.
Comment 3 Filip Pizlo 2016-09-28 13:31:45 PDT
(In reply to comment #2)
> Comment on attachment 290107 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290107&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2053
> > +            if (!fence->write && !fence->read)
> >                  return;
> 
> Does this happen in practice? Seems strange to specify a no-op fence. Maybe
> we can prohibit it.

It's weird, but I think it's harmless to allow it and potentially harmful to prohibit it.

I can imagine writing a transformation that somehow removes something from all heap ranges in some part of the B3 IR.  Maybe some phase proves that some HeapRange can't actually be written to or read from by some mass of code.  Such a phase might render some Fence's read or write range empty, since if it removes a range that is a superset of those ranges, then the ranges will become empty.  I believe that this is sound: if the program really doesn't care about some abstract heap and a fence is meant to protect motion of accesses in that abstract heap, then it is correct to weaken the fence.  But this means that the fence may be weakened all the way to the no-op fence.

You could argue that it's such an optimization's job to then remove the Fence.  But in my experience, it's not a good idea to do these mandatory optimizations, where the discovery of an optimization opportunity means that the compiler is obligated to do the optimization.  It buys you nothing but asserts that aren't catching any bugs.
Comment 4 Filip Pizlo 2016-09-28 13:33:24 PDT
Landed in https://trac.webkit.org/changeset/206539