[turbofan] Add CalleeSavedFPRegisters to CallDescriptor.

[turbofan] Add CalleeSavedFPRegisters to CallDescriptor.

This change makes possible to save and restore the FP registers
in the Prologue and Return parts for the CallAddress kind functions.

TEST=test-simplified-lowering/RunNumberDivide_2_TruncatingToUint32,
     test-simplified-lowering/RunNumberMultiply_TruncatingToUint32

Committed: https://crrev.com/b4275a70eb1e6b6dc0f3c77b9be6bcfca3f80c20
Cr-Commit-Position: refs/heads/master@{#29378}

[paul.l..., 2015-06-15 23:05:16]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-06-15 23:05:17]

This is going in the right direction, but please see the naming issue in
comments below. You will have to modify code-generator-xxx for all arch's.

https://codereview.chromium.org/1191513003/diff/1/src/compiler/arm/linkage-ar...
File src/compiler/arm/linkage-arm.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/arm/linkage-ar...
src/compiler/arm/linkage-arm.cc:26: static RegList CCalleeSaveFPURegisters() {
We should not use FPU in the names, that is MIPS specific. I first was thinking
CCalleeSaveDoubleRegisters(), as that is very generic. However, arm64 uses 'FP',
and I like that, plus the names don't get insanely long:
CCalleeSaveFPRegisters()

https://codereview.chromium.org/1191513003/diff/1/src/compiler/arm64/linkage-...
File src/compiler/arm64/linkage-arm64.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/arm64/linkage-...
src/compiler/arm64/linkage-arm64.cc:28: return 0;
arm64 has CPURegList::GetCallerSaved() & CPURegList::GetCallerSavedFP, and they
also have MacroAssembler::PushCPURegList(). You'll want to be careful that
whatever methods you use for save/restore are no slower than the stp/ldp
(register pair) operations that they use in the existing TF code.

https://codereview.chromium.org/1191513003/diff/1/src/compiler/mips/code-gene...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:1071: register_save_area_size +=
kPointerSize;
Rather than computing this, use kNumCalleeSaved + kNumCallSavedFPU (from
frames-mips.h) if that is included here. Else maybe move it as a function into
macro-assem, so we can avoid this for loop. Same below.

https://codereview.chromium.org/1191513003/diff/1/src/compiler/ppc/linkage-pp...
File src/compiler/ppc/linkage-ppc.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/ppc/linkage-pp...
src/compiler/ppc/linkage-ppc.cc:29: // TODO(palfia): finish.
I think the ppc callee saved are d14-d31.However, I have doubt, as they are not
saved in JSEntryStub.

https://codereview.chromium.org/1191513003/diff/1/src/compiler/x64/linkage-x6...
File src/compiler/x64/linkage-x64.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/x64/linkage-x6...
src/compiler/x64/linkage-x64.cc:39: }
Per JSEntryStub, xmm6 thru xmm15 are callee-saved, but _only_ on WIN64.

[akos.palfi.imgtec, 2015-06-22 17:33:20]

Thanks for the review, I've uploaded the ports (except PPC). PTAL. Testing is in
progress.

https://codereview.chromium.org/1191513003/diff/1/src/compiler/arm/linkage-ar...
File src/compiler/arm/linkage-arm.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/arm/linkage-ar...
src/compiler/arm/linkage-arm.cc:26: static RegList CCalleeSaveFPURegisters() {
On 2015/06/15 23:05:16, paul.l... wrote:
> We should not use FPU in the names, that is MIPS specific. I first was
thinking
> CCalleeSaveDoubleRegisters(), as that is very generic. However, arm64 uses
'FP',
> and I like that, plus the names don't get insanely long:
> CCalleeSaveFPRegisters()

Done.

https://codereview.chromium.org/1191513003/diff/1/src/compiler/arm64/linkage-...
File src/compiler/arm64/linkage-arm64.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/arm64/linkage-...
src/compiler/arm64/linkage-arm64.cc:28: return 0;
On 2015/06/15 23:05:17, paul.l... wrote:
> arm64 has CPURegList::GetCallerSaved() & CPURegList::GetCallerSavedFP, and
they
> also have MacroAssembler::PushCPURegList(). You'll want to be careful that
> whatever methods you use for save/restore are no slower than the stp/ldp
> (register pair) operations that they use in the existing TF code.

OK, I'm generating the same stp/ldp code.

https://codereview.chromium.org/1191513003/diff/1/src/compiler/mips/code-gene...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:1071: register_save_area_size +=
kPointerSize;
On 2015/06/15 23:05:17, paul.l... wrote:
> Rather than computing this, use kNumCalleeSaved + kNumCallSavedFPU (from
> frames-mips.h) if that is included here. Else maybe move it as a function into
> macro-assem, so we can avoid this for loop. Same below.

Done.

https://codereview.chromium.org/1191513003/diff/1/src/compiler/ppc/linkage-pp...
File src/compiler/ppc/linkage-ppc.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/ppc/linkage-pp...
src/compiler/ppc/linkage-ppc.cc:29: // TODO(palfia): finish.
On 2015/06/15 23:05:17, paul.l... wrote:
> I think the ppc callee saved are d14-d31.However, I have doubt, as they are
not
> saved in JSEntryStub.

Acknowledged.

https://codereview.chromium.org/1191513003/diff/1/src/compiler/x64/linkage-x6...
File src/compiler/x64/linkage-x64.cc (right):

https://codereview.chromium.org/1191513003/diff/1/src/compiler/x64/linkage-x6...
src/compiler/x64/linkage-x64.cc:39: }
On 2015/06/15 23:05:17, paul.l... wrote:
> Per JSEntryStub, xmm6 thru xmm15 are callee-saved, but _only_ on WIN64. 

Done.

[paul.l..., 2015-06-23 02:15:03]

nit: also change the title to CalleeSavedFPRegisters

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/arm64/code...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1081: // Save registers.
This looks a bit messy to me, but luckily there are arm64 macro-asm function to
do most of this, similar to the way we do on mips with MultiPush/Pop. I'll send
a complete diff separately, but the basic save code can look like this:

    CPURegList callee_save_fp =
        CPURegList(CPURegister::kFPRegister, kDRegSizeInBits,
                   descriptor->CalleeSavedFPRegisters());
    DCHECK(callee_save_fp.list() == CPURegList::GetCalleeSavedFP().list());
    int saved_count = callee_save_fp.Count();
    __ PushCPURegList(callee_save_fp);

    CPURegList callee_save = CPURegList(CPURegister::kRegister, kXRegSizeInBits,
                                        descriptor->CalleeSavedRegisters());
    // TODO(): TF save list not in sync with CPURegList::GetCalleeSaved() - x30
    // DCHECK(callee_save.list() == CPURegList::GetCalleeSaved().list());
    saved_count += callee_save.Count();
    __ PushCPURegList(callee_save);

    frame()->SetRegisterSaveAreaSize(saved_count * kPointerSize);

the 2nd DCHECK is a problem since the original TF code saves link-reg (X30), but
the assembler CPURegList::GetCalleeSaved() does not. I have a workaround I'll
send separately, but when we publish this, the arm team can help get the best
approach.

I don't think there is any perf issue here, as their Push/Pop macros do convert
into stp/ldp.

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/mips/code-...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/mips/code-...
src/compiler/mips/code-generator-mips.cc:1070: DCHECK(kNumCalleeSaved ==
base::bits::CountPopulation32(saves));
I really _like_ these DCHECKs to tie the new list of CalleeSavedRegisters() back
to the older implementation. So that if either is changed, that will get
noticed.

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/mips64/cod...
File src/compiler/mips64/code-generator-mips64.cc (right):

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/mips64/cod...
src/compiler/mips64/code-generator-mips64.cc:1141: if (saves != 0) {  // Save
callee-saved registers.
I think we should remove this if clause, here and 3 places below. First, they
won't be zero, and removing this improves readability. Second, the macro-asm
functions do no harm with a zero reglist, but they do emit one useless
instruction. That should be fixed in separate CL for mips & mips64.

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/ppc/linkag...
File src/compiler/ppc/linkage-ppc.cc (right):

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/ppc/linkag...
src/compiler/ppc/linkage-ppc.cc:29: // TODO(palfia): finish.
Remove the comment for now, and let's just return 0 here, per Matt B.

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:1525: __ movdqu(Operand(rsp, 16 *
slot_idx), XMMRegister::from_code(i));
Swap params here, per our slack discussion.

[akos.palfi.imgtec, 2015-06-23 19:48:57]

Comments addressed, thanks!

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/arm64/code...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1081: // Save registers.
On 2015/06/23 02:15:02, paul.l... wrote:
> This looks a bit messy to me, but luckily there are arm64 macro-asm function
to
> do most of this, similar to the way we do on mips with MultiPush/Pop. I'll
send
> a complete diff separately, but the basic save code can look like this:
> 
>     CPURegList callee_save_fp =
>         CPURegList(CPURegister::kFPRegister, kDRegSizeInBits,
>                    descriptor->CalleeSavedFPRegisters());
>     DCHECK(callee_save_fp.list() == CPURegList::GetCalleeSavedFP().list());
>     int saved_count = callee_save_fp.Count();
>     __ PushCPURegList(callee_save_fp);
> 
>     CPURegList callee_save = CPURegList(CPURegister::kRegister,
kXRegSizeInBits,
>                                         descriptor->CalleeSavedRegisters());
>     // TODO(): TF save list not in sync with CPURegList::GetCalleeSaved() -
x30
>     // DCHECK(callee_save.list() == CPURegList::GetCalleeSaved().list());
>     saved_count += callee_save.Count();
>     __ PushCPURegList(callee_save);
> 
>     frame()->SetRegisterSaveAreaSize(saved_count * kPointerSize);
> 
> the 2nd DCHECK is a problem since the original TF code saves link-reg (X30),
but
> the assembler CPURegList::GetCalleeSaved() does not. I have a workaround I'll
> send separately, but when we publish this, the arm team can help get the best
> approach.
> 
> I don't think there is any perf issue here, as their Push/Pop macros do
convert
> into stp/ldp.

Thanks, it's much cleaner now.

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/mips64/cod...
File src/compiler/mips64/code-generator-mips64.cc (right):

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/mips64/cod...
src/compiler/mips64/code-generator-mips64.cc:1141: if (saves != 0) {  // Save
callee-saved registers.
On 2015/06/23 02:15:02, paul.l... wrote:
> I think we should remove this if clause, here and 3 places below. First, they
> won't be zero, and removing this improves readability. Second, the macro-asm
> functions do no harm with a zero reglist, but they do emit one useless
> instruction. That should be fixed in separate CL for mips & mips64.

Done.

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/ppc/linkag...
File src/compiler/ppc/linkage-ppc.cc (right):

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/ppc/linkag...
src/compiler/ppc/linkage-ppc.cc:29: // TODO(palfia): finish.
On 2015/06/23 02:15:02, paul.l... wrote:
> Remove the comment for now, and let's just return 0 here, per Matt B.

Done.

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/1191513003/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:1525: __ movdqu(Operand(rsp, 16 *
slot_idx), XMMRegister::from_code(i));
On 2015/06/23 02:15:02, paul.l... wrote:
> Swap params here, per our slack discussion.

Done.

[paul.l..., 2015-06-23 20:07:01]

LGTM. Please publish, and add v8-team and arm-team reviewers. If you could also
make it clear in commit message that MIPS had these errrors due to callee-saved
FP being missed, and that it appears arm needs this too.

https://codereview.chromium.org/1191513003/diff/40001/src/compiler/arm64/code...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1191513003/diff/40001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1082: CPURegList callee_save_fp =
nit: change these vars to saves_fp, and saves, to match the other arch's. This
will have small readability benefit as you (or clang-format) can move following
function up to this line using 2 lines total.

https://codereview.chromium.org/1191513003/diff/40001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1153: CPURegList callee_save_fp =
nit: same here.

[akos.palfi.imgtec, 2015-06-23 20:33:19]

Done.

https://codereview.chromium.org/1191513003/diff/40001/src/compiler/arm64/code...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1191513003/diff/40001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1082: CPURegList callee_save_fp =
On 2015/06/23 20:07:00, paul.l... wrote:
> nit: change these vars to saves_fp, and saves, to match the other arch's. This
> will have small readability benefit as you (or clang-format) can move
following
> function up to this line using 2 lines total.

Done.

https://codereview.chromium.org/1191513003/diff/40001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1153: CPURegList callee_save_fp =
On 2015/06/23 20:07:00, paul.l... wrote:
> nit: same here.

Done.

[akos.palfi.imgtec, 2015-06-23 20:36:00]

The CQ bit was checked by akos.palfi@imgtec.com to run a CQ dry run

[akos.palfi.imgtec, 2015-06-23 20:36:00]

The patchset sent to the CQ was uploaded after l-g-t-m from paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1191513003/#ps80001
(title: "Nits.")

[commit-bot: I haz the power, 2015-06-23 20:36:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191513003/80001

[akos.palfi.imgtec, 2015-06-23 20:42:15]

akos.palfi@imgtec.com changed reviewers:
+ bmeurer@chromium.org, danno@chromium.org, jacob.bramley@arm.com,
rodolph.perfetta@arm.com, ulan@chromium.org

[akos.palfi.imgtec, 2015-06-23 20:42:16]

Fixes a register corruption problem where TF generated code which is called
directly from C overwrites a FP callee-saved register. We found that only the
arm64 port saves the callee-saved FP registers correctly. But the other archs
should also. We implemented a generic way to save FP regs for all archs,
following the integer callee-saves defined in linkage. PTAL. These two cctests
clearly triggers the reg corruption problem, but only on MIPS.

[akos.palfi.imgtec, 2015-06-23 20:54:33]

akos.palfi@imgtec.com changed reviewers:
+ balazs.kilvady@imgtec.com, dusan.milosavljevic@imgtec.com,
gergely.kis@imgtec.com

[commit-bot: I haz the power, 2015-06-23 21:04:35]

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

[commit-bot: I haz the power, 2015-06-23 21:04:37]

Dry run: Try jobs failed on following builders:
  v8_win64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel/builds/6012)

[Benedikt Meurer, 2015-06-24 07:04:21]

Looking good to me. Win64 seems to be unhappy tho.

https://codereview.chromium.org/1191513003/diff/80001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/1191513003/diff/80001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:954:
DCHECK(DwVfpRegister::kMaxNumRegisters == 32);
Nit: How about a static assert here?

https://codereview.chromium.org/1191513003/diff/80001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:957: DCHECK((last - first + 1) ==
base::bits::CountPopulation32(saves_fp));
Nit: How about DCHECK_EQ?

https://codereview.chromium.org/1191513003/diff/80001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1011:
DCHECK(DwVfpRegister::kMaxNumRegisters == 32);
Nit: STATIC_ASSERT?

[akos.palfi.imgtec, 2015-06-24 15:08:10]

The CQ bit was checked by akos.palfi@imgtec.com to run a CQ dry run

[akos.palfi.imgtec, 2015-06-24 15:08:11]

The patchset sent to the CQ was uploaded after l-g-t-m from paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1191513003/#ps100001
(title: "Fix win64 and nits.")

[commit-bot: I haz the power, 2015-06-24 15:08:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191513003/100001

[akos.palfi.imgtec, 2015-06-24 15:36:05]

Thanks for the review. I've addressed the nits and fixed the win64 build. PTAL.

https://codereview.chromium.org/1191513003/diff/80001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/1191513003/diff/80001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:954:
DCHECK(DwVfpRegister::kMaxNumRegisters == 32);
On 2015/06/24 07:04:21, Benedikt Meurer wrote:
> Nit: How about a static assert here?

Done.

https://codereview.chromium.org/1191513003/diff/80001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:957: DCHECK((last - first + 1) ==
base::bits::CountPopulation32(saves_fp));
On 2015/06/24 07:04:21, Benedikt Meurer wrote:
> Nit: How about DCHECK_EQ?

Done.

https://codereview.chromium.org/1191513003/diff/80001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1011:
DCHECK(DwVfpRegister::kMaxNumRegisters == 32);
On 2015/06/24 07:04:21, Benedikt Meurer wrote:
> Nit: STATIC_ASSERT?

Done.

[commit-bot: I haz the power, 2015-06-24 15:39:21]

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

[commit-bot: I haz the power, 2015-06-24 15:39:23]

Dry run: This issue passed the CQ dry run.

[akos.palfi.imgtec, 2015-06-26 12:05:37]

Rebased on master HEAD (af4c4b0427c44e74520d61f328f258b365ba233d).

[akos.palfi.imgtec, 2015-06-26 16:26:13]

The CQ bit was checked by akos.palfi@imgtec.com to run a CQ dry run

[akos.palfi.imgtec, 2015-06-26 16:26:13]

The patchset sent to the CQ was uploaded after l-g-t-m from paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1191513003/#ps120001
(title: "Rebased on bleeding_edge (af4c4b04).")

[commit-bot: I haz the power, 2015-06-26 16:26:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191513003/120001

[commit-bot: I haz the power, 2015-06-26 17:00:43]

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

[commit-bot: I haz the power, 2015-06-26 17:00:44]

Dry run: This issue passed the CQ dry run.

[jbramley, 2015-06-29 13:53:09]

LGTM (ARM + ARM64)

Thanks!

I think we should (later) investigate the saving of x30 (lr) for ARM64 but
you've covered this with a TODO and the behaviour is unchanged.
(Push/PopCalleeSavedRegisters also preserve lr, though they probably shouldn't.)

https://codereview.chromium.org/1191513003/diff/120001/src/compiler/arm64/cod...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1191513003/diff/120001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:1108: //
CPURegList::GetCalleeSaved(): x30 is missing.
x30 (lr) is caller-saved in the procedure-call standard, so we don't really need
to save it for the PCS alone. (ARM doesn't save its LR so I presume that TF
doesn't have a special need for it.) However, we do need to save an even number
of X registers here so perhaps we save it to bump us up to an even number.

[Benedikt Meurer, 2015-06-30 05:01:29]

LGTM

[akos.palfi.imgtec, 2015-06-30 12:31:49]

Thanks for the explanation of the x30 (lr) usage. In this case I'll leave this
TODO as is and if you think you can later remove/modify that in a follow-up CL.

https://codereview.chromium.org/1191513003/diff/120001/src/compiler/arm64/cod...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1191513003/diff/120001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:1108: //
CPURegList::GetCalleeSaved(): x30 is missing.
On 2015/06/29 13:53:09, jbramley wrote:
> x30 (lr) is caller-saved in the procedure-call standard, so we don't really
need
> to save it for the PCS alone. (ARM doesn't save its LR so I presume that TF
> doesn't have a special need for it.) However, we do need to save an even
number
> of X registers here so perhaps we save it to bump us up to an even number.

Acknowledged.

[akos.palfi.imgtec, 2015-06-30 12:32:17]

The CQ bit was checked by akos.palfi@imgtec.com

[commit-bot: I haz the power, 2015-06-30 12:32:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191513003/120001

[commit-bot: I haz the power, 2015-06-30 13:22:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-30 13:23:03]

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