Introduce BUILTIN_CALL_PAIR.

Introduce BUILTIN_CALL_PAIR.

This change allows the PPC simulator to execute on PPC hardware where,
due to calling conventions, we must distinguish between Object* and
ObjectPair return values.

We find this useful as another available option for debugging certain
problems.  While not strictly necessary for Intel platforms, we hope
that this is less offensive now that BUILTIN_CALL_TRIPLE has been
added.

BUG=
R=rmcilroy@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com

Committed: https://crrev.com/433e8848df4c7ec11b7b4a808f329f4a1e31142b
Cr-Commit-Position: refs/heads/master@{#33475}

[MTBrandyberry, 2016-01-19 22:50:39]

The CQ bit was checked by mbrandy@us.ibm.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 22:50:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1604653006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1604653006/20001

[MTBrandyberry, 2016-01-19 23:16:21]

PTAL

[commit-bot: I haz the power, 2016-01-19 23:25:22]

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

[commit-bot: I haz the power, 2016-01-19 23:25:22]

Dry run: This issue passed the CQ dry run.

[JoranSiu, 2016-01-20 14:59:01]

On 2016/01/19 23:25:22, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

The changes lgtm, but I'm not familiar with why BUILTIN_CALL_PAIR was removed
previously.  Running sim on S390 will have the same challenge, due to ABI
differences with returning ObjectPair structs on 64-bit.

[MTBrandyberry, 2016-01-20 17:13:39]

On 2016/01/20 14:59:01, JoranSiu wrote:
> On 2016/01/19 23:25:22, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> The changes lgtm, but I'm not familiar with why BUILTIN_CALL_PAIR was removed
> previously.  Running sim on S390 will have the same challenge, due to ABI
> differences with returning ObjectPair structs on 64-bit.

It wasn't removed... it was not previously needed due to the set of simulator
targets and the fact that it traditionally runs on Intel only.

I should note that this change allows the simulator to execute the generated
code without modification.  An alternate solution could have generated different
code in the simulated case, but I would argue that is counter to the purpose of
simulation.

[rmcilroy, 2016-01-21 12:15:33]

Looks reasonable to me. One comment and I guess you'll need to fix the other
simulators too. I didn't look at the PPC changes.

https://codereview.chromium.org/1604653006/diff/20001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1604653006/diff/20001/src/assembler.cc#newcod...
src/assembler.cc:1036: : (f->result_size == 2 ? BUILTIN_CALL_PAIR :
BUILTIN_CALL))) {}
Please pull this logic out into a helper function rather than nesting tertiary
operators.

[MTBrandyberry, 2016-01-21 14:41:37]

On 2016/01/21 12:15:33, rmcilroy wrote:
> Looks reasonable to me. One comment and I guess you'll need to fix the other
> simulators too. I didn't look at the PPC changes.
> 
> https://codereview.chromium.org/1604653006/diff/20001/src/assembler.cc
> File src/assembler.cc (right):
> 
>
https://codereview.chromium.org/1604653006/diff/20001/src/assembler.cc#newcod...
> src/assembler.cc:1036: : (f->result_size == 2 ? BUILTIN_CALL_PAIR :
> BUILTIN_CALL))) {}
> Please pull this logic out into a helper function rather than nesting tertiary
> operators.

Thanks for the review.  I did update the other sims such that the new pair type
is treated identically to the default -- as before.  I will upload a patch set
for your comment above.

[MTBrandyberry, 2016-01-21 16:00:32]

https://codereview.chromium.org/1604653006/diff/20001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1604653006/diff/20001/src/assembler.cc#newcod...
src/assembler.cc:1036: : (f->result_size == 2 ? BUILTIN_CALL_PAIR :
BUILTIN_CALL))) {}
On 2016/01/21 12:15:33, rmcilroy wrote:
> Please pull this logic out into a helper function rather than nesting tertiary
> operators.

Done.

[MTBrandyberry, 2016-01-21 17:56:12]

The CQ bit was checked by mbrandy@us.ibm.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 17:56:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1604653006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1604653006/40001

[commit-bot: I haz the power, 2016-01-21 18:18:10]

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

[commit-bot: I haz the power, 2016-01-21 18:18:11]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-01-22 11:13:52]

https://codereview.chromium.org/1604653006/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1604653006/diff/40001/src/assembler.h#newcode...
src/assembler.h:1047: static Type builtin_call_type(int size) {
/s/size/result_size

https://codereview.chromium.org/1604653006/diff/40001/src/assembler.h#newcode...
src/assembler.h:1047: static Type builtin_call_type(int size) {
Just call this BuiltinCallTypeForResultSize and make it a static helper function
in assembler.cc (it doesn't give the call type for other builtins like compare,
etc.)

https://codereview.chromium.org/1604653006/diff/40001/src/assembler.h#newcode...
src/assembler.h:1052: break;
nit - just "return BUILTIN_CALL" directly, no need for the type variable (or
break). For the other cases too.

[MTBrandyberry, 2016-01-22 14:53:53]

https://codereview.chromium.org/1604653006/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1604653006/diff/40001/src/assembler.h#newcode...
src/assembler.h:1047: static Type builtin_call_type(int size) {
On 2016/01/22 11:13:52, rmcilroy wrote:
> /s/size/result_size

Done.

https://codereview.chromium.org/1604653006/diff/40001/src/assembler.h#newcode...
src/assembler.h:1047: static Type builtin_call_type(int size) {
On 2016/01/22 11:13:52, rmcilroy wrote:
> /s/size/result_size

Done.

https://codereview.chromium.org/1604653006/diff/40001/src/assembler.h#newcode...
src/assembler.h:1052: break;
On 2016/01/22 11:13:52, rmcilroy wrote:
> nit - just "return BUILTIN_CALL" directly, no need for the type variable (or
> break). For the other cases too. 

Done.

[rmcilroy, 2016-01-22 17:18:49]

lgtm

[MTBrandyberry, 2016-01-22 17:40:00]

The CQ bit was checked by mbrandy@us.ibm.com

[MTBrandyberry, 2016-01-22 17:40:01]

The patchset sent to the CQ was uploaded after l-g-t-m from joransiu@ca.ibm.com
Link to the patchset: https://codereview.chromium.org/1604653006/#ps60001
(title: "Changes based on review comments.")

[commit-bot: I haz the power, 2016-01-22 17:40:05]

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

[commit-bot: I haz the power, 2016-01-22 18:34:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-22 18:35:48]

Description was changed from

==========
Introduce BUILTIN_CALL_PAIR.

This change allows the PPC simulator to execute on PPC hardware where,
due to calling conventions, we must distinguish between Object* and
ObjectPair return values.

We find this useful as another available option for debugging certain
problems.  While not strictly necessary for Intel platforms, we hope
that this is less offensive now that BUILTIN_CALL_TRIPLE has been
added.

BUG=
R=rmcilroy@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com,
michael_dawson@ca.ibm.com
==========

to

==========
Introduce BUILTIN_CALL_PAIR.

This change allows the PPC simulator to execute on PPC hardware where,
due to calling conventions, we must distinguish between Object* and
ObjectPair return values.

We find this useful as another available option for debugging certain
problems.  While not strictly necessary for Intel platforms, we hope
that this is less offensive now that BUILTIN_CALL_TRIPLE has been
added.

BUG=
R=rmcilroy@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com,
michael_dawson@ca.ibm.com

Committed: https://crrev.com/433e8848df4c7ec11b7b4a808f329f4a1e31142b
Cr-Commit-Position: refs/heads/master@{#33475}
==========

[commit-bot: I haz the power, 2016-01-22 18:35:48]

Patchset 4 (id:??) landed as
https://crrev.com/433e8848df4c7ec11b7b4a808f329f4a1e31142b
Cr-Commit-Position: refs/heads/master@{#33475}