Inline mathematical constants

Inline mathematical constants

Where possible, mathematical constant references
such as Math.PI are replaced with HConstant of their
actual value.

R=verwaest@chromium.org
BUG=

[Toon Verwaest, 2014-03-21 14:53:48]

No LGTM.

We inline read-only properties of global constants using a more general
mechanism.

[Toon Verwaest, 2014-03-21 14:54:07]

With no lgtm, I meant not lgtm.

[p.antonov, 2014-03-21 15:01:08]

On 2014/03/21 14:53:48, Toon Verwaest wrote:
> No LGTM.
> 
> We inline read-only properties of global constants using a more general
> mechanism.

It doesn't look like the mechanism is active for Math.PI and
others. Also from the top of my head I don't know any other cases. Just to
make sure, this is entirely different from e.g. undefined and NaN. One
can do Math.PI = {1.24}; for example.

[p.antonov, 2014-03-21 15:01:51]

> can do Math.PI = {1.24}; for example.

err. I mean Math = {PI: 1.24};

[kbalazs, 2014-03-21 16:15:16]

What is the performance impact of this patch?

[Toon Verwaest, 2014-03-21 17:22:47]

Sure, the optimization I'm suggesting isn't there yet. It's just "the right
thing to do".

Your implementation is not only pretty hacky, but also wrong. E.g., you only
deoptimize while overwriting the property through the runtime. An assignment via
IC or Crankshaft would not trap, and would lead to wrong semantics.

We don't want to pollute all paths of the code for such a "simple optimization"
that can be automatically covered by a wider applicable optimization.

[p.antonov, 2014-03-21 18:28:56]

On 2014/03/21 17:22:47, Toon Verwaest wrote:
> Sure, the optimization I'm suggesting isn't there yet. It's just "the right
> thing to do".

Right. I would say this patch could also be generalized to support the
case of writable global objects with constant properties. However, outside
Math.*
there isn't any interesting uses afaik.
 
> Your implementation is not only pretty hacky, but also wrong. E.g., you only
> deoptimize while overwriting the property through the runtime. An assignment
via
> IC or Crankshaft would not trap, and would lead to wrong semantics.

I think doing it with inserted guards would negate all or a lot of the
performance gained.

Can you explain how this is different from an assignment to the array or the
object prototype
elements which is also only checked in runtime to deoptimize the
kElementsCantBeAddedGroup?

[Toon Verwaest, 2014-03-24 13:04:11]

The AllowUndefinedAsNaN optimization is pretty tricky, and more extensive than
just kElementsCantBeAdded.

kElementsCantBeAdded will trigger when SetFastElement is applied to exactly
Array.prototype. We won't be able to warm up an IC for Array.prototype using a
different object, since it has a unique map. The same goes for Crankshaft.
(Unlike in your case).

Additionally, if you do something like Array.prototype.length = 1, it'll
deoptimize due to a prototype chain change, since it will make the map go from
FAST_SMI_ELEMENTS to FAST_HOLEY_SMI_ELEMENTS. The prototype-chain check is
added, but will most likely be elided and replaced by prototype dependency.

[p.antonov, 2014-03-24 13:19:57]

I see, so it has to be done with inserted guards then, or do you have a better
idea?

In addition, the patch should also be generalized so that it works for all
similar cases
e.g. Number.MAX_VALUE.

Is there any other major issue other than these two?

[Toon Verwaest, 2014-03-24 13:26:50]

So I still don't suggest you do this work as you are doing it here, but rather
as I described before.

Whenever you are generating a load from a read-only non-configurable property,
and you know the receiver (because it's an HConstant), just inline the resulting
value. That should cover all properties you are trying to inline here. Math is
most likely a global constant, so it'll get inlined. And all properties such as
PI are read-only non-configurable properties.

[Toon Verwaest, 2014-03-24 13:28:11]

Also, you don't need to add any guards, you just need to extend load-handling in
crankshaft to support loading read-only non-configurable properties from
HConstants. Everything else is taken care of already.

[p.antonov, 2014-03-24 14:19:13]

On 2014/03/24 13:28:11, Toon Verwaest wrote:
> Also, you don't need to add any guards, you just need to extend load-handling
in
> crankshaft to support loading read-only non-configurable properties from
> HConstants. Everything else is taken care of already.

I see what you mean, thank you. I will be able to work on this patch again on
next week.

[Toon Verwaest, 2014-03-24 16:19:19]

I closed this issue. Please upload a new patch if you try the other approach.