Hey guys, thanks for all your feedback! Most things have been addressed,
though
a couple are still up for further discussion.
This is NOT ready for landing yet, still need to rebase to today and add
Benedikt's float32 constant change. And I have one small bug still to chase.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-generator-mips.cc
File src/compiler/mips/code-generator-mips.cc (right):
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-generator-mips.cc#newcode108
src/compiler/mips/code-generator-mips.cc:108: // messy macro?
Consider expanding this in place for sll, srl, sra ops.
On 2014/09/24 15:23:31, titzer wrote:
I agree that you should kill this macro here.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-generator-mips.cc#newcode170
src/compiler/mips/code-generator-mips.cc:170: i.InputOperand(1),
kCompareReg, kScratchReg);
On 2014/09/24 15:23:31, titzer wrote:
The macro assembler methods here generate a lot of instructions. It
may not be
an overall win to generate an add with overflow on MIPS, but instead
to generate
an explicit tag check with a branch and a regular add.
Yes, Overflow checking is an issue for MIPS. We have two variations of
add/sub, one generates TRAP on overflow[1], the other silently
overflows. We use the silent version, but must generate extra
instructions to check. I don't understand your suggestion "generate an
explicit tag check with a branch and a regular add", can you elaborate?
The new MIPS r6 instruction set gives us a new/faster way to detect
overflows, we will implement that soon, but still need the old way for
older cores.
[1] I prototyped with the TRAP version a year ago, and it made a nice
speed improvement on some benchmarks, but I thought it best to avoid
installing signal handler, it could be fragile, and maybe problematic
with sandboxing.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-generator-mips.cc#newcode361
src/compiler/mips/code-generator-mips.cc:361: FlagsCondition condition)
{
Complications arise from MIPS not having condition codes like other
arch's. The compare and branch are merged into a single instruction,
which means that the branch instruction needs the registers provided to
the compare to still be valid. The current FlagsContinuation mechanism
does guarantee this. As you can see we mostly don't emit instructions
for the compare itself, but we do preserve the type of comparison as a
psuedo-op to guide the branch generation.
The handling of branches here is tough to follow.
Yes, the code is a bit ugly :( It "looked better" here with single-line
cases:
https://github.com/paul99/v8m-rb/blob/b8e8df9/src/compiler/mips/code-generator-mips.cc#L403
but clang-format won't take that form. I'm thinking of splitting into 4
smaller routines, each specialized for one arch_opcode type. I'd be glad
to hear your ideas.
Another issue is the exposed branch delay slots here, which make things
faster at the expense of readability.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-codes-mips.h
File src/compiler/mips/instruction-codes-mips.h (right):
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-codes-mips.h#newcode34
src/compiler/mips/instruction-codes-mips.h:34: V(MipsFloat64Cmp)
\
On 2014/09/25 05:37:10, Benedikt Meurer wrote:
On 2014/09/24 at 15:23:32, titzer wrote:
> Consider naming these opcodes as close as possible to the underlying
Assembler/MacroAssembler method that they end up calling. We didn't do
that on
arm but I think we want to clean that up in the future.
Yep, it's on my TODO list for ARM.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc
File src/compiler/mips/instruction-selector-mips-unittest.cc (right):
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode20
src/compiler/mips/instruction-selector-mips-unittest.cc:20:
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
You should add std::ostream& operator<<(std::ostream&, const
MachInst<T>&) for
this, otherwise test failures will be hard to interpret.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode37
src/compiler/mips/instruction-selector-mips-unittest.cc:37: static const
FPCmp kFPCmpInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode66
src/compiler/mips/instruction-selector-mips-unittest.cc:66: static const
MachInst2 kLogicalInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode80
src/compiler/mips/instruction-selector-mips-unittest.cc:80: static const
MachInst2 kShiftInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode96
src/compiler/mips/instruction-selector-mips-unittest.cc:96: static const
MachInst2 kMulDivInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode111
src/compiler/mips/instruction-selector-mips-unittest.cc:111: static
const MachInst2 kModInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode123
src/compiler/mips/instruction-selector-mips-unittest.cc:123: static
const MachInst2 kFPArithInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode135
src/compiler/mips/instruction-selector-mips-unittest.cc:135: static
const MachInst2 kAddSubInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode149
src/compiler/mips/instruction-selector-mips-unittest.cc:149: static
const MachInst1 kAddSubOneInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode161
src/compiler/mips/instruction-selector-mips-unittest.cc:161: static
const IntCmp kCmpInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode194
src/compiler/mips/instruction-selector-mips-unittest.cc:194: static
const Conversion kConversionInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: no need to use static, it's in an anonymous namespace already.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode512
src/compiler/mips/instruction-selector-mips-unittest.cc:512:
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
You should add
std::ostream& operator<<(std::ostream& os, const MemoryAccessImm& acc)
{
OStringStream ost;
ost << acc.type;
return os << ost.c_str();
}
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode522
src/compiler/mips/instruction-selector-mips-unittest.cc:522:
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Stream operator, see above.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode529
src/compiler/mips/instruction-selector-mips-unittest.cc:529: static
const MemoryAccessImm kMemoryAccessesImm[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: static, see above.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode581
src/compiler/mips/instruction-selector-mips-unittest.cc:581: static
const MemoryAccessImm1 kMemoryAccessImmMoreThan16bit[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: static, see above.
Done.
https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instruction-selector-mips-unittest.cc#newcode795
src/compiler/mips/instruction-selector-mips-unittest.cc:795:
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
Nit: extra new line :-)
Done.
https://codereview.chromium.org/601723002/diff/20001/src/mips/code-stubs-mips.cc
File src/mips/code-stubs-mips.cc (right):
https://codereview.chromium.org/601723002/diff/20001/src/mips/code-stubs-mips.cc#newcode1103
src/mips/code-stubs-mips.cc:1103: // a1: pointer to builtin function
On 2014/09/24 15:23:32, titzer wrote:
Changes here and other changes outside of TurboFan already look fine
(i.e. ready
to land).
Would you consider splitting those out into a separate CL and landing
them
first?
Done.
https://codereview.chromium.org/601723002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
