Some comments. Adding Sven for more opinions. Generally, this patch opens
up a
bunch of questions, and I'm not familiar enough with ARM to answer all of
these:
- Do we support crankshaft for VFPv2 already, or only for VFPv3?
- Is there a documentation with a full list of what instructions are
available
on VFPv3 and not on VFPv2?
- Have the assertions in all VFP instructions been consequently updated to
the
correct requirement (VFPv2 or VFPv3)? Or is this only the first of many CLs?
https://chromiumcodereview.appspot.com/10796069/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):
https://chromiumcodereview.appspot.com/10796069/diff/1/src/arm/assembler-arm.cc#newcode1748
src/arm/assembler-arm.cc:1748: ASSERT(CpuFeatures::IsEnabled(VFP3) ||
CpuFeatures::IsEnabled(VFP2));
it seems to me that VFP3 is a superset of VFP2, VSTR is part of VFP2 and
CpuFeatures::IsEnabled(VFP3) implies CpuFeatures::IsEnabled(VFP3). So I
would like to suggest removing the redundant
CpuFeatures::IsEnabled(VFP3).
https://chromiumcodereview.appspot.com/10796069/diff/1/src/arm/assembler-arm.cc#newcode2020
src/arm/assembler-arm.cc:2020: ASSERT(CpuFeatures::IsEnabled(VFP3));
From what I found, this is also available for VFPv2?
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/CJAEFGHE.html
https://chromiumcodereview.appspot.com/10796069/diff/1/src/arm/assembler-arm.cc#newcode2034
src/arm/assembler-arm.cc:2034: ASSERT(CpuFeatures::IsEnabled(VFP3) ||
CpuFeatures::IsEnabled(VFP2));
Ditto.
https://chromiumcodereview.appspot.com/10796069/diff/1/src/arm/assembler-arm.cc#newcode2049
src/arm/assembler-arm.cc:2049: ASSERT(CpuFeatures::IsEnabled(VFP3));
Ditto. Also enable for VFP2?
https://chromiumcodereview.appspot.com/10796069/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
