Bug 162342

Summary: Optimize B3->Air lowering of Fence on ARM
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jfbastien, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 162343    
Bug Blocks: 162316    
Attachments:
Description Flags
the patch ggaren: review+

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