MIPS: Saving Hydrogen code stubs in the snapshot requires us to respect the availability of VFP32DR…

MIPS: Saving Hydrogen code stubs in the snapshot requires us to respect the availability of VFP32DREGS.

Port r14338 (c4cfeda0)

BUG=

[Paul Lind, 2013-04-18 23:26:28]

Perhaps I am missing the intent of the original CL, but I do not think this
applies to MIPS. Now that soft-float is deprecated on ARM and MIPS, I believe we
always need to save FP regs. But on ARM, there is a question of "how many",
since some cores have 16 d-regs, and others have 32 d-regs.

In Michael's CL this expression: "CpuFeatures::IsSafeForSnapshot(VFP32DREGS) ?
kSaveFPRegs : kDontSaveFPRegs" evaluates to kDontSaveFPRegs if there are only 16
d- regs. So in StoreBufferOverflowStub::Generate() NO d-regs would get saved.

That does not quite make sense to me, maybe Michael or you can explain what I am
missing here.

MIPS does not have a variable number, so your similar expression will always
evaluate to kSaveFPRegs, which is what's in the code now. So it won't hurt us to
delay porting this a day, until we understand.

[mvstanton, 2013-04-19 07:08:50]

You probably don't need this on MIPS. We needed something like this on ia32 for
SSE2. The intent of that change (a few weeks ago) was to avoid saving FP regs in
the snapshot, not only because we don't yet know if we have SSE2, but also
because the snapshot usages of the record write stub code didn't need to save FP
regs for the stores.

Paul, thanks also for noticing the logical error in the use of VFP32DREGS! I
really wanted to avoid saving FP regs at all. A more logical fix might be to
replace the IsSafeForSnapshot() statement with !Serializer::enabled(). I will
follow up on the question of soft-float...indeed, maybe I do need to always save
FP regs on ARM, and therefore need another solution for the snapshot.

On 2013/04/18 23:26:28, Paul Lind wrote:
> Perhaps I am missing the intent of the original CL, but I do not think this
> applies to MIPS. Now that soft-float is deprecated on ARM and MIPS, I believe
we
> always need to save FP regs. But on ARM, there is a question of "how many",
> since some cores have 16 d-regs, and others have 32 d-regs.
> 
> In Michael's CL this expression: "CpuFeatures::IsSafeForSnapshot(VFP32DREGS) ?
> kSaveFPRegs : kDontSaveFPRegs" evaluates to kDontSaveFPRegs if there are only
16
> d- regs. So in StoreBufferOverflowStub::Generate() NO d-regs would get saved.
> 
> That does not quite make sense to me, maybe Michael or you can explain what I
am
> missing here.
> 
> MIPS does not have a variable number, so your similar expression will always
> evaluate to kSaveFPRegs, which is what's in the code now. So it won't hurt us
to
> delay porting this a day, until we understand.

[mvstanton, 2013-04-19 07:08:50]

You probably don't need this on MIPS. We needed something like this on ia32 for
SSE2. The intent of that change (a few weeks ago) was to avoid saving FP regs in
the snapshot, not only because we don't yet know if we have SSE2, but also
because the snapshot usages of the record write stub code didn't need to save FP
regs for the stores.

Paul, thanks also for noticing the logical error in the use of VFP32DREGS! I
really wanted to avoid saving FP regs at all. A more logical fix might be to
replace the IsSafeForSnapshot() statement with !Serializer::enabled(). I will
follow up on the question of soft-float...indeed, maybe I do need to always save
FP regs on ARM, and therefore need another solution for the snapshot.

On 2013/04/18 23:26:28, Paul Lind wrote:
> Perhaps I am missing the intent of the original CL, but I do not think this
> applies to MIPS. Now that soft-float is deprecated on ARM and MIPS, I believe
we
> always need to save FP regs. But on ARM, there is a question of "how many",
> since some cores have 16 d-regs, and others have 32 d-regs.
> 
> In Michael's CL this expression: "CpuFeatures::IsSafeForSnapshot(VFP32DREGS) ?
> kSaveFPRegs : kDontSaveFPRegs" evaluates to kDontSaveFPRegs if there are only
16
> d- regs. So in StoreBufferOverflowStub::Generate() NO d-regs would get saved.
> 
> That does not quite make sense to me, maybe Michael or you can explain what I
am
> missing here.
> 
> MIPS does not have a variable number, so your similar expression will always
> evaluate to kSaveFPRegs, which is what's in the code now. So it won't hurt us
to
> delay porting this a day, until we understand.

[mvstanton, 2013-04-19 08:16:07]

Hi, it's a bug to not save the fp regs (my bad). I'm working on a CL that will
save them all. The trick is that I will need to save either 16 or 32 depending
on runtime conditions. The code we generate at snapshot time can't count on
having 32, but it will run later in an environment where we have 32, and a call
into C code for example, might clobber 32.

On 2013/04/19 07:08:50, mvstanton wrote:
> You probably don't need this on MIPS. We needed something like this on ia32
for
> SSE2. The intent of that change (a few weeks ago) was to avoid saving FP regs
in
> the snapshot, not only because we don't yet know if we have SSE2, but also
> because the snapshot usages of the record write stub code didn't need to save
FP
> regs for the stores.
> 
> Paul, thanks also for noticing the logical error in the use of VFP32DREGS! I
> really wanted to avoid saving FP regs at all. A more logical fix might be to
> replace the IsSafeForSnapshot() statement with !Serializer::enabled(). I will
> follow up on the question of soft-float...indeed, maybe I do need to always
save
> FP regs on ARM, and therefore need another solution for the snapshot.
> 
> On 2013/04/18 23:26:28, Paul Lind wrote:
> > Perhaps I am missing the intent of the original CL, but I do not think this
> > applies to MIPS. Now that soft-float is deprecated on ARM and MIPS, I
believe
> we
> > always need to save FP regs. But on ARM, there is a question of "how many",
> > since some cores have 16 d-regs, and others have 32 d-regs.
> > 
> > In Michael's CL this expression: "CpuFeatures::IsSafeForSnapshot(VFP32DREGS)
?
> > kSaveFPRegs : kDontSaveFPRegs" evaluates to kDontSaveFPRegs if there are
only
> 16
> > d- regs. So in StoreBufferOverflowStub::Generate() NO d-regs would get
saved.
> > 
> > That does not quite make sense to me, maybe Michael or you can explain what
I
> am
> > missing here.
> > 
> > MIPS does not have a variable number, so your similar expression will always
> > evaluate to kSaveFPRegs, which is what's in the code now. So it won't hurt
us
> to
> > delay porting this a day, until we understand.

[mvstanton, 2013-04-19 16:04:42]

Hi Paul, I checked in an update that fixes the issue. I'm not sure if it's
useful to MIPS or not, take a look if you like:
https://codereview.chromium.org/14246032/

Thx again,
--Michael

[palfia, 2013-04-19 16:34:18]

Hi guys,

Thanks for the detailed explanation of this problem. I'm closing this issue,
since it doesn't apply to MIPS (but at least it helped to find a hidden bug :)
).