Bug 162342 – Optimize B3->Air lowering of Fence on ARM

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 290107 [details]
the patch

[Geoffrey Garen]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/206539