ARM: Enable the subnormal floats on QNX.

API: Add v8::V8::PerformPerThreadSetup.

This method needs to be invoked by the embedder on QNX/ARM,
in order to enable the subnormal float support. On the other
platforms, this method is a no-op.

BUG=

Patch from Cosmin Truta <ctruta@blackberry.com>;

[c.truta, 2014-01-06 15:59:05]

Subnormal floats aren't enabled by default on QNX/ARM, so I enabled them
explicitly, in this patch.

[Benedikt Meurer, 2014-01-07 06:27:33]

We are moving the thread creation stuff out of V8 and into Blink/Chromium.
Please talk to jochen@chromium.org about incorporating your changes for QNX as
this is currently work-in-progress.

[c.truta, 2014-01-07 09:02:49]

On 2014/01/07 06:27:33, Benedikt Meurer wrote:
> We are moving the thread creation stuff out of V8 and into Blink/Chromium.
> Please talk to mailto:jochen@chromium.org about incorporating your changes for
QNX as
> this is currently work-in-progress.

Will do, thank you very much.

[Sven Panne, 2014-01-07 09:48:48]

Quick drive-by comments...

https://codereview.chromium.org/125133002/diff/1/src/platform-qnx.cc
File src/platform-qnx.cc (right):

https://codereview.chromium.org/125133002/diff/1/src/platform-qnx.cc#newcode116
src/platform-qnx.cc:116: asm volatile("vmsr fpscr, %0" : : "r"(fpscr));
I think a single parameter-less asm with 3 instructions would be more readable
than this asm/C++/asm dance.

https://codereview.chromium.org/125133002/diff/1/src/platform-qnx.cc#newcode123
src/platform-qnx.cc:123: } qnx_arm_static_initializer;
No new static initializers, please, use V8::InitializeOncePerProcessImpl when
really needed.

[jochen (gone - plz use gerrit), 2014-01-08 09:59:37]

the threads created by v8 don't execute javascript, are you aware of this?

[c.truta, 2014-01-08 12:39:35]

On 2014/01/08 09:59:37, jochen wrote:
> the threads created by v8 don't execute javascript, are you aware of this?

I wasn't aware, thank you. I thought some of them are created for web workers,
as it's done in JavaScriptCore, for example.

We need denormals for JS-executing threads only, be it the main thread, or
worker threads. I will take one more look and find a correct place to put this
initialization.

Or, if thread creation and passing between Blink and V8 is still changing, I can
wait with this patch, until things stabilize.

[jochen (gone - plz use gerrit), 2014-01-08 12:42:19]

On 2014/01/08 12:39:35, cosmin.truta wrote:
> On 2014/01/08 09:59:37, jochen wrote:
> > the threads created by v8 don't execute javascript, are you aware of this?
> 
> I wasn't aware, thank you. I thought some of them are created for web workers,
> as it's done in JavaScriptCore, for example.
> 
> We need denormals for JS-executing threads only, be it the main thread, or
> worker threads. I will take one more look and find a correct place to put this
> initialization.
> 
> Or, if thread creation and passing between Blink and V8 is still changing, I
can
> wait with this patch, until things stabilize.

in any case, the creation of threads for js execution is done by a V8 embedder
not by V8.

Maybe if you enter an Isolate on a thread, you can store in the TLS whether
subnormal floats were already enabled on this thread and if not enable them?

[jochen (gone - plz use gerrit), 2014-01-08 12:42:54]

On 2014/01/08 12:42:19, jochen wrote:
> On 2014/01/08 12:39:35, cosmin.truta wrote:
> > On 2014/01/08 09:59:37, jochen wrote:
> > > the threads created by v8 don't execute javascript, are you aware of this?
> > 
> > I wasn't aware, thank you. I thought some of them are created for web
workers,
> > as it's done in JavaScriptCore, for example.
> > 
> > We need denormals for JS-executing threads only, be it the main thread, or
> > worker threads. I will take one more look and find a correct place to put
this
> > initialization.
> > 
> > Or, if thread creation and passing between Blink and V8 is still changing, I
> can
> > wait with this patch, until things stabilize.
> 
> in any case, the creation of threads for js execution is done by a V8 embedder
> not by V8.
> 
> Maybe if you enter an Isolate on a thread, you can store in the TLS whether
> subnormal floats were already enabled on this thread and if not enable them?

But really, V8 shouldn't use TLS, and the embedder should take care of enabling
subnormal floats.

[c.truta, 2014-01-08 12:59:23]

On 2014/01/08 12:42:54, jochen wrote:
> But really, V8 shouldn't use TLS, and the embedder should take care of
enabling
> subnormal floats.

Not even in the main V8 thread?

For example, should that code be in src/d8.cc, samples/shell.cc, test/cctest? I
would be inclined to respond 'no', although we need subnormals in the
command-line tools and in cctest also.

Jochen, would you be okay if we add this initialization in
V8::InitializeOncePerProcessImpl, as Sven originally suggested, and then I'll
look for the right place in Chromium where worker threads are created? (I don't
know that place yet, I still need to familiarize myself with it.)

[Sven Panne, 2014-01-08 13:08:03]

On 2014/01/08 12:59:23, cosmin.truta wrote:
> On 2014/01/08 12:42:54, jochen wrote:
> [...] Jochen, would you be okay if we add this initialization in
> V8::InitializeOncePerProcessImpl, as Sven originally suggested, and then I'll
> look for the right place in Chromium where worker threads are created? (I
don't
> know that place yet, I still need to familiarize myself with it.)

Reconsidering this, I think it should really be done only on the embedding side,
not in V8::InitializeOncePerProcessImpl. d8, shell, etc. are just embedders, and
it's not v8's business to do some funny per-thread initialization. Setting some
(thread-)global state is not nice in general when somehing is embedded
elsewhere...

[c.truta, 2014-01-08 13:18:03]

On 2014/01/08 13:08:03, Sven Panne wrote:
> Reconsidering this, I think it should really be done only on the embedding
side,
> not in V8::InitializeOncePerProcessImpl. d8, shell, etc. are just embedders,
and
> it's not v8's business to do some funny per-thread initialization. Setting
some
> (thread-)global state is not nice in general when somehing is embedded
> elsewhere...

Ok, Sven. Does this mean you would be okay with adding the QNX-specific
initialization asm to d8/shell and cctest instead? If yes, could you please
advise where would it be best to put it?

Or, would you prefer that we not do it there at all, either?

[Sven Panne, 2014-01-08 13:47:17]

On 2014/01/08 13:18:03, cosmin.truta wrote:
> Ok, Sven. Does this mean you would be okay with adding the QNX-specific
> initialization asm to d8/shell and cctest instead? If yes, could you please
> advise where would it be best to put it?

Well, not directly into d8, shell, cctest etc., probably put something like
OS::DoFunnyPerThreadInit() into the platform-* files and use that, avoiding
duplication and #ifdefs. But I would like to hear from others what they think
about this...
 
> Or, would you prefer that we not do it there at all, either?

Hmmm, then d8 etc. won't really work on QNX AFACIT, so this doesn't seem to be
an option.

[c.truta, 2014-01-08 14:20:35]

On 2014/01/08 13:47:17, Sven Panne wrote:
> Hmmm, then d8 etc. won't really work on QNX AFACIT, so this doesn't seem to be
> an option.

Sven, I truly appreciate how you guys are trying to accommodate us.

Back to this topic: it is an option, meaning that we can maintain a separate set
of patches which we can apply (and also publish) separately. We, of course, aim
towards as few such private patches as necessary; ideally, zero. We are,
nonetheless, adapting ourselves to everyone else's requirements also, in the
best way we can. And, yes, we need both d8 and cctest to work correctly.

So I had initially proposed adding the initialization code into our own
platform-qnx.cc, out of everyone else's way. Even if that means adding one more
static initializer, it may be a bit of pain, but for ourselves only. On the
other hand, if we were to add that to d8 and cctest, there should be duplicated
code, and it will be in everyone's sight.

Whatever you end up deciding, I will follow it, we will be fine :-)

[Sven Panne, 2014-01-08 14:41:15]

On 2014/01/08 14:20:35, cosmin.truta wrote:
> [...] So I had initially proposed adding the initialization code into our own
> platform-qnx.cc, out of everyone else's way. Even if that means adding one
more
> static initializer, it may be a bit of pain, but for ourselves only. On the
> other hand, if we were to add that to d8 and cctest, there should be
duplicated
> code, and it will be in everyone's sight. [...]

My proposal was to add something like 

   void OS::DoFunnyPerThreadInit() { <funky QNX asm magic>... }

to platform-qnx.cc, and dummy functions

   void OS::DoFunnyPerThreadInit() { }

to other platform-*.cc files, and call this from d8, shell, etc. This way there
is no duplication, no #ifdefs, and the thread stuff is done where it belongs,
namely in the embedder.

But as I said, I'd like to hear from the other reviewers what their proposal is.

[jochen (gone - plz use gerrit), 2014-01-08 15:00:52]

On 2014/01/08 14:41:15, Sven Panne wrote:
> On 2014/01/08 14:20:35, cosmin.truta wrote:
> > [...] So I had initially proposed adding the initialization code into our
own
> > platform-qnx.cc, out of everyone else's way. Even if that means adding one
> more
> > static initializer, it may be a bit of pain, but for ourselves only. On the
> > other hand, if we were to add that to d8 and cctest, there should be
> duplicated
> > code, and it will be in everyone's sight. [...]
> 
> My proposal was to add something like 
> 
>    void OS::DoFunnyPerThreadInit() { <funky QNX asm magic>... }
> 
> to platform-qnx.cc, and dummy functions
> 
>    void OS::DoFunnyPerThreadInit() { }
> 
> to other platform-*.cc files, and call this from d8, shell, etc. This way
there
> is no duplication, no #ifdefs, and the thread stuff is done where it belongs,
> namely in the embedder.
> 
> But as I said, I'd like to hear from the other reviewers what their proposal
is.

I would put it somewhere where the embedder can call it

Similar to how the embedder needs to invoke InitializeICU() before using v8.

[c.truta, 2014-01-22 10:55:40]

On 2014/01/08 15:00:52, jochen wrote:
> I would put it somewhere where the embedder can call it
> Similar to how the embedder needs to invoke InitializeICU() before using v8.

Done. Is this what you had in mind, Jochen? (See Patch Set 2.)

I named this method V8::PerThreadSetUp and OS::PerThreadSetUp, akin to
OS::PostSetUp. Sven suggested OS::DoFunnyPerThreadInit, and I hope it's okay
with this shorter name instead.

I reworded the commit message also.

[jochen (gone - plz use gerrit), 2014-01-22 11:26:19]

yes, that's what I head in mind, thanks!

https://codereview.chromium.org/125133002/diff/200001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/125133002/diff/200001/include/v8.h#newcode4740
include/v8.h:4740: static void PerThreadSetUp();
I'd prefer a name of the form Verb+Action, e.g. PerformPerThreadSetup()

[c.truta, 2014-01-22 12:26:37]

On 2014/01/22 11:26:19, jochen wrote:
> https://codereview.chromium.org/125133002/diff/200001/include/v8.h#newcode4740
> include/v8.h:4740: static void PerThreadSetUp();
> I'd prefer a name of the form Verb+Action, e.g. PerformPerThreadSetup()

Done, thank you very much.

[jochen (gone - plz use gerrit), 2014-01-22 13:43:49]

so currently, background threads of V8 won't enable this option. Is this ok?
AFAIK they don't execute compiled javascript, so I guess it doesn't matter

[c.truta, 2014-01-22 14:18:08]

On 2014/01/22 13:43:49, jochen wrote:
> so currently, background threads of V8 won't enable this option. Is this ok?
> AFAIK they don't execute compiled javascript, so I guess it doesn't matter

Right. It matters in a few specific places (JavaScript, CSS, graphics, etc.)
where FP calculations are performed and we enable them specifically to handle
the very small numbers. Outside these areas, it doesn't make a difference.

[jochen (gone - plz use gerrit), 2014-01-22 14:21:37]

lgtm (but I'm not an owner)

[danno, 2014-01-22 15:36:21]

You are now :-)

[Sven Panne, 2014-01-28 07:16:27]

Anything still missing before we can land this?

[jochen (gone - plz use gerrit), 2014-01-28 13:11:26]

no, i think this can be landed

[Benedikt Meurer, 2014-03-31 06:57:46]

On 2014/01/28 13:11:26, jochen wrote:
> no, i think this can be landed

Do we actually want to land this?

[Sven Panne, 2014-06-13 11:25:06]

On 2014/03/31 06:57:46, Benedikt Meurer wrote:
> On 2014/01/28 13:11:26, jochen wrote:
> > no, i think this can be landed
> 
> Do we actually want to land this?

+1 for either landing or closing this CL...

[c.truta, 2014-06-16 17:15:45]

On 2014/06/13 11:25:06, Sven Panne wrote:
> +1 for either landing or closing this CL...

This CL needs rebasing. I tried to update it, but "git cl upload" failed
with "No module named checkdeps". I see checkdeps is removed from Chromium,
and I suppose PRESUBMIT.py needs to be updated accordingly.

[c.truta, 2014-06-16 17:47:21]

On 2014/06/16 17:15:45, c.truta wrote:
> On 2014/06/13 11:25:06, Sven Panne wrote:
> > +1 for either landing or closing this CL...
> 
> This CL needs rebasing. I tried to update it, but "git cl upload" failed
> with "No module named checkdeps". I see checkdeps is removed from Chromium,
> and I suppose PRESUBMIT.py needs to be updated accordingly.

I checked out an older version of Chromium so that PRESUBMIT.py can access
checkdeps, and I submitted the rebased CL.

[Michael Starzinger, 2014-06-16 22:02:09]

On 2014/06/16 17:47:21, c.truta wrote:
> On 2014/06/16 17:15:45, c.truta wrote:
> > On 2014/06/13 11:25:06, Sven Panne wrote:
> > > +1 for either landing or closing this CL...
> > 
> > This CL needs rebasing. I tried to update it, but "git cl upload" failed
> > with "No module named checkdeps". I see checkdeps is removed from Chromium,
> > and I suppose PRESUBMIT.py needs to be updated accordingly.
> 
> I checked out an older version of Chromium so that PRESUBMIT.py can access
> checkdeps, and I submitted the rebased CL.

Just run "make dependencies" from tip-of-tree V8 to fix this.

[c.truta, 2014-06-16 23:09:09]

On 2014/06/16 22:02:09, Michael Starzinger wrote:
> On 2014/06/16 17:47:21, c.truta wrote:
> > I checked out an older version of Chromium so that PRESUBMIT.py can access
> > checkdeps, and I submitted the rebased CL.
> 
> Just run "make dependencies" from tip-of-tree V8 to fix this.

I see that now, thanks.

[Sven Panne, 2014-06-17 06:05:50]

@Rodolph: Could you take a quick look if this CL makes sense (esp.
src/platform.h)?

[Rodolph Perfetta, 2014-06-17 08:29:40]

lgtm