Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23496041:
ARM: EmitNumberUntagD() Improvment (Closed)
|
Created:
7 years, 3 months ago by Bangfu Modified:
7 years, 3 months ago CC:
v8-dev Visibility:
Public. |
DescriptionSave one branch for normal heap number un-tagging.
R=bmeurer@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=16856
Patch Set 1 : add if condition #Patch Set 2 : #
Total comments: 2
Patch Set 3 : indentation #Patch Set 4 : branch reordering #Messages
Total messages: 13 (0 generated)
Your CL upload is broken.
On 2013/09/12 07:06:10, Benedikt Meurer wrote: > Your CL upload is broken. It's OK now.
LGTM with nits. https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... File src/arm/lithium-codegen-arm.cc (right): https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... src/arm/lithium-codegen-arm.cc:4894: __ b(eq, &heap_number); Nit: indentation. https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... src/arm/lithium-codegen-arm.cc:4937: Nit: new line.
On 2013/09/12 07:23:40, Benedikt Meurer wrote: > LGTM with nits. > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > File src/arm/lithium-codegen-arm.cc (right): > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > src/arm/lithium-codegen-arm.cc:4894: __ b(eq, &heap_number); > Nit: indentation. > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > src/arm/lithium-codegen-arm.cc:4937: > Nit: new line. Nit: Updated.
On 2013/09/12 07:32:48, Bangfu wrote: > On 2013/09/12 07:23:40, Benedikt Meurer wrote: > > LGTM with nits. > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > File src/arm/lithium-codegen-arm.cc (right): > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > src/arm/lithium-codegen-arm.cc:4894: __ b(eq, &heap_number); > > Nit: indentation. > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > src/arm/lithium-codegen-arm.cc:4937: > > Nit: new line. > > Nit: Updated. hei, I really like the idea of this patch, since e.g. on x64 i verified that we spend 10% of the untagging cpu cycles on this branch! but it took me a while to understand what your code was doing. maybe you could have a look at my port: https://codereview.chromium.org/23872026/. cheers
On 2013/09/13 09:15:23, oliv wrote: > On 2013/09/12 07:32:48, Bangfu wrote: > > On 2013/09/12 07:23:40, Benedikt Meurer wrote: > > > LGTM with nits. > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > File src/arm/lithium-codegen-arm.cc (right): > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > src/arm/lithium-codegen-arm.cc:4894: __ b(eq, &heap_number); > > > Nit: indentation. > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > src/arm/lithium-codegen-arm.cc:4937: > > > Nit: new line. > > > > Nit: Updated. > > hei, I really like the idea of this patch, since e.g. on x64 i verified that we > spend 10% of the untagging cpu cycles on this branch! but it took me a while to > understand what your code was doing. maybe you could have a look at my port: > https://codereview.chromium.org/23872026/. > > cheers Thanks for review.
On 2013/09/13 10:16:58, Bangfu wrote: > On 2013/09/13 09:15:23, oliv wrote: > > On 2013/09/12 07:32:48, Bangfu wrote: > > > On 2013/09/12 07:23:40, Benedikt Meurer wrote: > > > > LGTM with nits. > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > File src/arm/lithium-codegen-arm.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > src/arm/lithium-codegen-arm.cc:4894: __ b(eq, &heap_number); > > > > Nit: indentation. > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > src/arm/lithium-codegen-arm.cc:4937: > > > > Nit: new line. > > > > > > Nit: Updated. > > > > hei, I really like the idea of this patch, since e.g. on x64 i verified that > we > > spend 10% of the untagging cpu cycles on this branch! but it took me a while > to > > understand what your code was doing. maybe you could have a look at my port: > > https://codereview.chromium.org/23872026/. > > > > cheers > > Thanks for review. Do you want me to close this issue?
On 2013/09/18 07:20:46, Bangfu wrote: > On 2013/09/13 10:16:58, Bangfu wrote: > > On 2013/09/13 09:15:23, oliv wrote: > > > On 2013/09/12 07:32:48, Bangfu wrote: > > > > On 2013/09/12 07:23:40, Benedikt Meurer wrote: > > > > > LGTM with nits. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > File src/arm/lithium-codegen-arm.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > src/arm/lithium-codegen-arm.cc:4894: __ b(eq, &heap_number); > > > > > Nit: indentation. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > src/arm/lithium-codegen-arm.cc:4937: > > > > > Nit: new line. > > > > > > > > Nit: Updated. > > > > > > hei, I really like the idea of this patch, since e.g. on x64 i verified that > > we > > > spend 10% of the untagging cpu cycles on this branch! but it took me a while > > to > > > understand what your code was doing. maybe you could have a look at my port: > > > https://codereview.chromium.org/23872026/. > > > > > > cheers > > > > Thanks for review. > > Do you want me to close this issue? Sorry for the delay... No, can you please simplify your patch similar to what olivf@ did with the Intel patch. The most important point IMHO is to avoid the conditional branch on the common execution path.
On 2013/09/18 09:26:18, Benedikt Meurer wrote: > On 2013/09/18 07:20:46, Bangfu wrote: > > On 2013/09/13 10:16:58, Bangfu wrote: > > > On 2013/09/13 09:15:23, oliv wrote: > > > > On 2013/09/12 07:32:48, Bangfu wrote: > > > > > On 2013/09/12 07:23:40, Benedikt Meurer wrote: > > > > > > LGTM with nits. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > > File src/arm/lithium-codegen-arm.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > > src/arm/lithium-codegen-arm.cc:4894: __ b(eq, &heap_number); > > > > > > Nit: indentation. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > > src/arm/lithium-codegen-arm.cc:4937: > > > > > > Nit: new line. > > > > > > > > > > Nit: Updated. > > > > > > > > hei, I really like the idea of this patch, since e.g. on x64 i verified > that > > > we > > > > spend 10% of the untagging cpu cycles on this branch! but it took me a > while > > > to > > > > understand what your code was doing. maybe you could have a look at my > port: > > > > https://codereview.chromium.org/23872026/. > > > > > > > > cheers > > > > > > Thanks for review. > > > > Do you want me to close this issue? > > Sorry for the delay... No, can you please simplify your patch similar to what > olivf@ did with the Intel patch. The most important point IMHO is to avoid the > conditional branch on the common execution path. I've looked x64 version code, in common execution path it has: 1. smi check 2. heap number check 3. load value 4. conditional branch to convert 5. unconditional branch to done In our ARM patch, in common execution path it has: 1. smi check 2. heap number check 3. conditional load value 4. conditional branch to done
On 2013/09/18 10:28:14, Bangfu wrote: > On 2013/09/18 09:26:18, Benedikt Meurer wrote: > > On 2013/09/18 07:20:46, Bangfu wrote: > > > On 2013/09/13 10:16:58, Bangfu wrote: > > > > On 2013/09/13 09:15:23, oliv wrote: > > > > > On 2013/09/12 07:32:48, Bangfu wrote: > > > > > > On 2013/09/12 07:23:40, Benedikt Meurer wrote: > > > > > > > LGTM with nits. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > > > File src/arm/lithium-codegen-arm.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > > > src/arm/lithium-codegen-arm.cc:4894: __ b(eq, &heap_number); > > > > > > > Nit: indentation. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > > > src/arm/lithium-codegen-arm.cc:4937: > > > > > > > Nit: new line. > > > > > > > > > > > > Nit: Updated. > > > > > > > > > > hei, I really like the idea of this patch, since e.g. on x64 i verified > > that > > > > we > > > > > spend 10% of the untagging cpu cycles on this branch! but it took me a > > while > > > > to > > > > > understand what your code was doing. maybe you could have a look at my > > port: > > > > > https://codereview.chromium.org/23872026/. > > > > > > > > > > cheers > > > > > > > > Thanks for review. > > > > > > Do you want me to close this issue? > > > > Sorry for the delay... No, can you please simplify your patch similar to what > > olivf@ did with the Intel patch. The most important point IMHO is to avoid the > > conditional branch on the common execution path. > > I've looked x64 version code, in common execution path it has: > 1. smi check > 2. heap number check > 3. load value > 4. conditional branch to convert > 5. unconditional branch to done > > In our ARM patch, in common execution path it has: > 1. smi check > 2. heap number check > 3. conditional load value > 4. conditional branch to done Right. It'd be nice to move the conditional load out of the if's and replace the conditional branch to done with a conditional branch to convert (not taken in the common case) and the unconditional branch to done.
On 2013/09/18 10:32:17, Benedikt Meurer wrote: > On 2013/09/18 10:28:14, Bangfu wrote: > > On 2013/09/18 09:26:18, Benedikt Meurer wrote: > > > On 2013/09/18 07:20:46, Bangfu wrote: > > > > On 2013/09/13 10:16:58, Bangfu wrote: > > > > > On 2013/09/13 09:15:23, oliv wrote: > > > > > > On 2013/09/12 07:32:48, Bangfu wrote: > > > > > > > On 2013/09/12 07:23:40, Benedikt Meurer wrote: > > > > > > > > LGTM with nits. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > > > > File src/arm/lithium-codegen-arm.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > > > > src/arm/lithium-codegen-arm.cc:4894: __ b(eq, &heap_number); > > > > > > > > Nit: indentation. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23496041/diff/4002/src/arm/lithium-codegen-ar... > > > > > > > > src/arm/lithium-codegen-arm.cc:4937: > > > > > > > > Nit: new line. > > > > > > > > > > > > > > Nit: Updated. > > > > > > > > > > > > hei, I really like the idea of this patch, since e.g. on x64 i > verified > > > that > > > > > we > > > > > > spend 10% of the untagging cpu cycles on this branch! but it took me a > > > while > > > > > to > > > > > > understand what your code was doing. maybe you could have a look at my > > > port: > > > > > > https://codereview.chromium.org/23872026/. > > > > > > > > > > > > cheers > > > > > > > > > > Thanks for review. > > > > > > > > Do you want me to close this issue? > > > > > > Sorry for the delay... No, can you please simplify your patch similar to > what > > > olivf@ did with the Intel patch. The most important point IMHO is to avoid > the > > > conditional branch on the common execution path. > > > > I've looked x64 version code, in common execution path it has: > > 1. smi check > > 2. heap number check > > 3. load value > > 4. conditional branch to convert > > 5. unconditional branch to done > > > > In our ARM patch, in common execution path it has: > > 1. smi check > > 2. heap number check > > 3. conditional load value > > 4. conditional branch to done > > Right. It'd be nice to move the conditional load out of the if's and replace the > conditional branch to done with a conditional branch to convert (not taken in > the common case) and the unconditional branch to done. Done.
LGTM, will land, thx.
Message was sent while issue was closed.
Committed patchset #4 manually as r16856 (presubmit successful). |
