provides a mechanism for optimizing compilers to select the different Register configuration.

For some platform such as X87, Crankshaft and Turbofan needs to use different
    register configurations currently. This CL provides a mechanism so that
    optimizing compilers can select different Register Configuration.

BUG=

Committed: https://crrev.com/5978b926c607899752f515e291f28ac274be1f9f
Cr-Commit-Position: refs/heads/master@{#31476}

[chunyang.dai, 2015-10-14 15:22:06]

Patchset #1 (id:1) has been deleted

[chunyang.dai, 2015-10-15 08:20:14]

Patchset #2 (id:40001) has been deleted

[chunyang.dai, 2015-10-15 14:37:57]

Patchset #2 (id:60001) has been deleted

[chunyang.dai, 2015-10-15 14:38:53]

update the patch that we move the x87 platform specific change to
registerconfiguration module.

[chunyang.dai, 2015-10-15 14:43:15]

chunyang.dai@intel.com changed reviewers:
+ danno@chromium.org, titzer@chromium.org, weiliang.lin@intel.com

[chunyang.dai, 2015-10-15 14:43:15]

Danno & Titzer.
please review this new patch for the the x87 double register issue we discussed
during yesterday's meeting. 
we moved the platform specific code change to registerconfigration module.
please review it. 

thanks

[titzer, 2015-10-15 16:44:43]

https://codereview.chromium.org/1405673003/diff/80001/src/register-configurat...
File src/register-configuration.h (right):

https://codereview.chromium.org/1405673003/diff/80001/src/register-configurat...
src/register-configuration.h:38: #if V8_TARGET_ARCH_X87
Seems like this check should be moved to the ArchDefault() method.

[chunyang.dai, 2015-10-16 08:46:38]

hi, Titzer.

 Thank you very much for your review.
 
 According to my understanding, if we do the platform specific check in
ArchDefault, we have to
 create ArchDefaultRegisterConfiguration and
ArchDefaultRegisterConfigurationForTurbofan separately.
 in ArchDefault function, the RegisterConfiration instance is Readonly. we
cannot change the field
 value in it. so I have to create two classes with different parameters
separately for them.
 so this patchset touches more code than patchset2. 
 
 Anyway it's a temporary solution for us as I marked in the code comments. we
will unify double register
 number for x87 in the future. If we cannot fix this issue quickly, it's a big
effort for us to do many
 change in the codegen part and it will block us to porting the remaining
patches. 
 
 Thanks.

[danno, 2015-10-16 08:47:02]

https://codereview.chromium.org/1405673003/diff/100001/src/register-configura...
File src/register-configuration.cc (right):

https://codereview.chromium.org/1405673003/diff/100001/src/register-configura...
src/register-configuration.cc:39: ArchDefaultRegisterConfiguration()
If you pass the enum value mentioned elsewhere in my comments to the
constructor, you can have a single ArchDefaultRegisterConfiguration class with a
very local decision based on the parameter for x87 and removes the need to
duplicate all the code below.

https://codereview.chromium.org/1405673003/diff/100001/src/register-configura...
File src/register-configuration.h (right):

https://codereview.chromium.org/1405673003/diff/100001/src/register-configura...
src/register-configuration.h:21: static const RegisterConfiguration*
ArchDefault();
I would prefer to have a single ArchDefault call that has a enum { CRANKSHAFT,
TURBOFAN } parameter that returns one or the other configurations.

[chunyang.dai, 2015-10-16 13:06:07]

hello, Danno & Titizer.

I updated the patch. Please review it. 

thanks.

[danno, 2015-10-19 10:48:59]

https://codereview.chromium.org/1405673003/diff/120001/src/register-configura...
File src/register-configuration.h (right):

https://codereview.chromium.org/1405673003/diff/120001/src/register-configura...
src/register-configuration.h:14: enum JITCompiler { CRANKSHAFT, TURBOFAN };
Please put this inside the RegisterConfiguration class and call it
CompilerSelector.

https://codereview.chromium.org/1405673003/diff/120001/src/register-configura...
src/register-configuration.h:25: JITCompiler compiler = CRANKSHAFT);
Please don't specify a parameter default here, it should always be explicit.

[chunyang.dai, 2015-10-20 08:14:01]

Patchset #5 (id:140001) has been deleted

[chunyang.dai, 2015-10-20 08:16:33]

Danno.

Thanks for your review comments. 
I updated it according to your comments. PTAL.

thanks

[Weiliang, 2015-10-20 09:27:51]

On 2015/10/20 08:16:33, chunyang.dai wrote:
> Danno.
> 
> Thanks for your review comments. 
> I updated it according to your comments. PTAL.
> 
> thanks

@cy, 

Could you please split your fix update and code rebase into two CL? Otherwise,
reviewers need to review the whole patch again. 

Thanks
-Weiliang

[chunyang.dai, 2015-10-20 10:00:20]

hi,weiliang.

Except for pipeline.cc file, all other changes are only for this patch. All of
them are 
the ArchDefault() parameter change. 


Hi, Danno:

In order to reduce effort, is it ok that the rebase change in pipeline.cc file
are included
in this patch set?  

thanks.

[danno, 2015-10-20 10:21:04]

On 2015/10/20 at 10:00:20, chunyang.dai wrote:
> hi,weiliang.
> 
> Except for pipeline.cc file, all other changes are only for this patch. All of
them are 
> the ArchDefault() parameter change. 
> 
> 
> Hi, Danno:
> 
> In order to reduce effort, is it ok that the rebase change in pipeline.cc file
are included
> in this patch set?  
> 
> thanks.

If you mean if it's OK to do the refactor and the functionality change in a
single CL, I'd prefer to keep them separate, i.e. a first CL that just does the
refactor without behavior change (that means all configurations would use the
TurboFan 1-register config), and then a second CL that allows Crankshaft to use
more registers.

[chunyang.dai, 2015-10-20 11:35:21]

Description was changed from

==========
Currently X87 turbofan backend use only one double register. Turbofan use only
one double register.
    The reason is that the X87 register is stack-based and the register
allocator use the same algorithm
    for all registers. So the backend of X87 needs to maintain the X87 stack
status between basic blocks
    during code generation. But for Turbofan backend, it can only handle
discrete platform-related LIRs.
    It's difficult to maintain the x87 stack status during different basic
blocks.
    So we use this temporary solution for X87 under the same
RegisterConfiguration for turbofan/crankshaft.
    we tried to touch as less common code as possible. We will unify the
allocatable double registers for X87
   in the future.

BUG=
==========

to

==========
For some platform such as X87, Crankshaft and Turbofan needs to use different
    register configurations currently. This CL provides a mechanism so that 
    optimizing compilers can select different Register Configuration. 

BUG=
==========

[chunyang.dai, 2015-10-20 11:50:39]

hello, Danno.
  I changed it according to your suggestion. 

  This CL provides the mechanism that different optimizing compiler can select
  different register configuration.

  And in another CL: https://codereview.chromium.org/1413343002/ , it will
configure
  different allocatable double register number for X87 basing on the
CompilerSelector.


  PTAL.

thanks.

[danno, 2015-10-21 14:09:57]

Getting close...

https://codereview.chromium.org/1405673003/diff/160001/src/register-configura...
File src/register-configuration.h (right):

https://codereview.chromium.org/1405673003/diff/160001/src/register-configura...
src/register-configuration.h:19: enum CompilerSelector { CRANKSHAFT, TURBOFAN };
Could you please add a TODO here that this distinction in RegisterConfigurations
is temporary until x87 TF supports all of the registers that Crankshaft does?

[chunyang.dai, 2015-10-21 14:55:44]

Hello. Danno:
Thank you very much for your patient review and adopting this temporary solution
for X87.

I added the TODO comments according to your suggestion. PTAL.

And if this patchset is ok. Please help to review the follow-up CL of this
patch：
https://codereview.chromium.org/1413343002/.

Thanks.

[danno, 2015-10-22 12:34:58]

On 2015/10/21 at 14:55:44, chunyang.dai wrote:
> Hello. Danno:
> Thank you very much for your patient review and adopting this temporary
solution
> for X87.
> 
> I added the TODO comments according to your suggestion. PTAL.
> 
> And if this patchset is ok. Please help to review the follow-up CL of this
patch：
> https://codereview.chromium.org/1413343002/.
> 
> Thanks.

lgtm

[danno, 2015-10-22 12:35:02]

The CQ bit was checked by danno@chromium.org

[commit-bot: I haz the power, 2015-10-22 12:35:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405673003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405673003/200001

[commit-bot: I haz the power, 2015-10-22 12:38:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-22 12:38:19]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6944)

[Michael Starzinger, 2015-10-22 13:02:53]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2015-10-22 13:02:54]

LGTM (rubber-stamp for "compiler" directory, didn't look at the rest).

[chunyang.dai, 2015-10-22 13:18:07]

hi, Titzer:
Could you please review the latest patchset again?

Danno gave LGTM but this CL needs LGTM from the owners of ./src/compiler folder.

Thanks.

[chunyang.dai, 2015-10-22 13:18:58]

Sorry, I did not notice LGTM from Michael.

[chunyang.dai, 2015-10-22 13:21:53]

The CQ bit was checked by chunyang.dai@intel.com

[commit-bot: I haz the power, 2015-10-22 13:22:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405673003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405673003/200001

[commit-bot: I haz the power, 2015-10-22 13:24:34]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-10-22 13:25:00]

Patchset 7 (id:??) landed as
https://crrev.com/5978b926c607899752f515e291f28ac274be1f9f
Cr-Commit-Position: refs/heads/master@{#31476}