Open Bug 657806 Opened 14 years ago Updated 2 years ago

Turn off strict aliasing in JS engine

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

People

(Reporter: billm, Unassigned)

References

Details

I just ran SunSpider and V8 on Linux opt builds with -fstrict-aliasing and -fno-strict-aliasing. I'm unable to see any performance difference.

The advantage of turning it off is that we don't have to deal with lots of annoying warnings. Also, we won't get any more scary bugs like bug 567627.
Interesting!  I'm trying to turn it on in the main build in bug 414641.  (My patch also makes strict aliasing warnings errors.)

Good performance numbers are kind of hard to come by right now (bug 653961) -- maybe we can wait to see what the full-browser perf is like with and without strict aliasing before flipping the JS switch?
Also, did you benchmark using gcc 4.5?  The strict aliasing engine was overhauled for that version.  (It might be worth benchmarking pgo'ed builds, too.)
As I understand it, -fstrict-aliasing warnings highlight undefined behaviour. If that's the case (http://gcc.gnu.org/onlinedocs/gcc-4.4.3/gcc/Optimize-Options.html#index-fstrict_002daliasing-750 indicates it is), then I think we should keep -fstrict-aliasing turned on.
The problem is that the warning engine uses heuristics -- it's not guaranteed to warn about all undefined behavior.  It's therefore possible to get things like bug 567627 even with -Werror-strict-aliasing.
First, just to clarify what Justin said, -fstrict-aliasing controls what code gets generated, while -Wstrict-aliasing controls what warnings are printed. Here are the two cases under consideration:

-fstrict-aliasing -Wstrict-aliasing: GCC may generate faster code, but it may also print spurious warnings and it may generate "incorrect" code without warning you (by "incorrect" I mean code that's correct according to the C standard but doesn't do what you want). This last piece is what's scary, I think.

-fno-strict-aliasing: The generated code may be slower, but you will never get any warnings and you don't have to worry about incorrect code.

I think a reasonable middle ground would be to turn strict aliasing on for most of the browser but turn it off for JS. The reason I'd like to turn it off is that I'm pretty sure there's no way to write, say, a custom memory allocator that is correct according to the C standard. There's a ton of this sort of code in the JS engine, particularly the GC.

However, I definitely don't want to regress performance. I'll try GCC 4.5 when I get a chance.
One optimization strict aliasing allows is the re-ordering of reads and writes.  If I know that pointers X and Y can't point to the same thing (that is, they can't alias), then given code that does

  store *X
  load *Y

I can place the load before the store.  This might help me avoid pipeline stalls when I go to use the value *Y.

Modern desktop CPUs already do this kind of reordering internally, so it might not help much there.  But this kind of thing could matter more on ARM or Atom.

My point is that it might be pretty hard to establish whether this optimization has an effect on speed.  That might be a good reason to turn it off, though!  :)
I forgot to add: another reason to turn it off in the JS engine alone is that most of our execution time is spent in generated code. So compiler optimizations like this have less effect on total execution time.

Also, given the possibility for this optimization to make us crash unexpectedly, I think we should lean toward turning it off unless we can prove that it actually improves performance.
Strict aliasing is disabled for the main tree by default (bug 413253).

But it also appears to be disabled by default for the JS tree: http://mxr.mozilla.org/mozilla-central/source/js/src/configure.in#1579

So I'm not really sure what this bug is about.

Bug 414641 is about turning strict aliasing back on.
(In reply to comment #9)
> But it also appears to be disabled by default for the JS tree:
> http://mxr.mozilla.org/mozilla-central/source/js/src/configure.in#1579

It's a bit confusing. Here is where it's turned on:
http://mxr.mozilla.org/mozilla-central/source/js/src/Makefile.in#90

The combination of these two causes all JS files to be compiled with both -fno-strict-aliasing and -fstrict-aliasing. The second one wins, maybe because it comes last.
(In reply to comment #6)
> -fstrict-aliasing -Wstrict-aliasing: GCC may generate faster code, but it
> may also print spurious warnings and it may generate "incorrect" code
> without warning you (by "incorrect" I mean code that's correct according to
> the C standard but doesn't do what you want).

Just to be clear: C99 and C++98 both have strict aliasing clauses (C++'s is 3.10.15) so, IIUC, whatever the compiler is allowed to do with -fstrict-aliasing its technically allowed to do with -fno-strict-aliasing; its just that it chooses not to for obvious reasons.  However, I still do not agree with comment 3 since, currently, we seem to getting the worst of both worlds; false negatives and false positives.  I could be forgetting, but I don't think I've seen a true positive yet.
Bug 421984 added this on purpose for the JS engine.
Blocks: 421984
> I could be forgetting, but I don't think I've seen a true positive yet.

I definitely saw some true positives in bug 414641.  There's at least one false negative, although if that's all, I think that's pretty impressive.  (With that one issue fixed, the browser passes all the tests.  But there's of course a fear that it might still be broken.)
Strict aliasing was a win back when we spent a lot more time executing C++ than we do now. I can see it not mattering these days.
Based on the discussion in bug 680515, I'd like to try and resurrect this bug if possible. Justin, it looked like things kind of petered out in bug 414641. Would you be more amenable to turning strict aliasing off in JS now?

To restate the case: there are a number of places in the JS engine where we knowingly and somewhat unavoidably violate the language's aliasing rules (particularly in the GC). On rare occasions, this leads to correctness issues like bug 567627. More commonly, we end up with lots of warnings and we have to hack around them with some creative casting. If we turn off -fstrict-aliasing, these problems will go away in practice (although I realize that we're still technically breaking the rules). Given that we haven't been able to reliably measure any performance gain from having strict aliasing enabled, and given the headaches it causes, I think it should be disabled.
Rafael, does LLVM support disabling strict aliasing and is that consistently respected during PGO etc?
> Justin, it looked like things kind of petered out in bug 414641. Would you be 
> more amenable to turning strict aliasing off in JS now?

Yes, I would be.  To recap bug 414641, I had GCC generating working code with -fstrict-aliasing on for the browser, but it broke when we turned on -O3.  From my point of view, this is a strong argument against using -fstrict-aliasing anywhere -- your code could break whenever GCC changes how it optimizes code.  It even seems possible that the nondeterminism in PGO could cause some builds to work and others to be broken.

Since strict aliasing violations are so common, I have to imagine that all current and future toolchains will support -fno-strict-aliasing.  Also, any compiler which doesn't support -fno-strict-aliasing will apparently have difficulty with the browser.  (Not to suggest it's not worth checking, of course!)
(In reply to Andreas Gal :gal from comment #16)
> Rafael, does LLVM support disabling strict aliasing and is that consistently
> respected during PGO etc?

LLVM has no PGO at the moment. In any case, the type alias rules are explicitly embedded in the IL, so -fno-strict-aliasing just omits that information and everything works as expected.
(In reply to Justin Lebar [:jlebar] (out 8/12 - 8/21) from comment #17)
> > Justin, it looked like things kind of petered out in bug 414641. Would you be 
> > more amenable to turning strict aliasing off in JS now?
> 
> Yes, I would be.  To recap bug 414641, I had GCC generating working code
> with -fstrict-aliasing on for the browser, but it broke when we turned on
> -O3.  From my point of view, this is a strong argument against using
> -fstrict-aliasing anywhere -- your code could break whenever GCC changes how
> it optimizes code.  It even seems possible that the nondeterminism in PGO
> could cause some builds to work and others to be broken.

We could make the PGO builds deterministic. The way I have seem it done before is to check in the profiles in the repository and update them from time to time.

I would love for our build to be deterministic. If other also think it is important, I can open a bug to track it.

> Since strict aliasing violations are so common, I have to imagine that all
> current and future toolchains will support -fno-strict-aliasing.  Also, any
> compiler which doesn't support -fno-strict-aliasing will apparently have
> difficulty with the browser.  (Not to suggest it's not worth checking, of
> course!)

We should check code size too.

I worry a bit about options that enable extensions to the standard language. Non strict aliasing is an extension, and one that is hard to know when it is being used.

Does VC have an option for turning strict aliasing off too?
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #19)
> Does VC have an option for turning strict aliasing off too?

Strict aliasing is off by default on MSVC.  I'm not aware of any way to turn it *on*.
> We could make the PGO builds deterministic. The way I have seem it done before 
> is to check in the profiles in the repository and update them from time to time.

Even if this were workable (I'm not sure it is, but we can discuss in another bug if you'd like), you'd still have the problem that, when updating the profiles, you might tickle the compiler into mis-optimizing.
Is anyone working on this? Can we close it as wontfix for now and let anyone that starts working on it again reopen it or create a new bug?
Assignee: general → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.