Hi, I've worked on your comments. The big work in this change. was to move the code from the Assembler to the MacroAssembler. There is also a new test for the assembler. However I actually use a MacroAssembler to use the Branch instructions. Otherwise only Assembler instructions are used.
I'll keep working on all this and start implementing basic code for functions,
as you said on the v8-users list. Regards, Alexandre. http://codereview.chromium.org/543161/diff/4001/5005 File src/bootstrapper.cc (right): http://codereview.chromium.org/543161/diff/4001/5005#newcode901 src/bootstrapper.cc:901: // Call function using either the runtime object or the global Reverted On 2010/01/26 22:40:55, Søren Gjesse wrote:
Please revert this to remove bootstrapper.cc from this change. I will
fix this
on bleeding_edge.
http://codereview.chromium.org/543161/diff/4001/5052 File src/execution.cc (right): http://codereview.chromium.org/543161/diff/4001/5052#newcode41 src/execution.cc:41: #elif V8_TARGET_ARCH_MIPS Ok. I don't have the simulator.h file yet. I'll change this as soon as I do. On 2010/01/26 22:40:55, Søren Gjesse wrote:
In bleeding_edge this #ifdef block have been moved to "simulator.h"
http://codereview.chromium.org/543161/diff/4001/5044 File src/mips/assembler-mips.cc (right): http://codereview.chromium.org/543161/diff/4001/5044#newcode1049 src/mips/assembler-mips.cc:1049: // Arithmetic I'll simplify all instructions and move the higher level to the macro-assembler. On 2010/01/26 22:40:55, Søren Gjesse wrote:
These arithmetic instructions should be split into assembler and
macro-assembler
versions. E.g. in assembler the addi instruction should ASSERT that
the
immediate is 16-bit and just emmit a ADDI instruction. The
macro-assembler
should then have an Add macro taking a 32-bit immediate.
http://codereview.chromium.org/543161/diff/4001/5044#newcode1303 src/mips/assembler-mips.cc:1303: Instr instr = SPECIAL | TGE | rs.code()<<21 | rt.code()<<16 | code<<6; They were implemented later than this code. I'll replace them. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Why not use the kXxxShift constants from constants-mips.h here and
below. http://codereview.chromium.org/543161/diff/4001/5044#newcode1408 src/mips/assembler-mips.cc:1408: void Assembler::ldc1(CRegister fd, const MemOperand& src) { Originally it wasn't. Anyway I have to remove the alignment stuff since the solution of emulating 4-byte alignment access is not ok. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Another macro-assembler macro.
http://codereview.chromium.org/543161/diff/4001/5044#newcode1443 src/mips/assembler-mips.cc:1443: void Assembler::sdc1(CRegister fd, const MemOperand& src) { Macro removed. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Ditto.
http://codereview.chromium.org/543161/diff/4001/5044#newcode1568 src/mips/assembler-mips.cc:1568: //------------Pseudo-instructions------------- Moved to the macro-assembler. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Pseudo-instructions are macros for the macro-assembler.
http://codereview.chromium.org/543161/diff/4001/5044#newcode1861 src/mips/assembler-mips.cc:1861: // nop(); Well it was more like a helper to remind which instruction we are generating. I change it to look more like comments. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Code in comments.
http://codereview.chromium.org/543161/diff/4001/5048 File src/mips/assembler-mips.h (right): http://codereview.chromium.org/543161/diff/4001/5048#newcode132 src/mips/assembler-mips.h:132: struct CRegister { Renamed. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Rename CRegister to FPURegister like in simulator.
http://codereview.chromium.org/543161/diff/4001/5048#newcode402 src/mips/assembler-mips.h:402: // instruction. Moved to macro-assembler. Renamed to Branch / BranchAndLink. On 2010/01/26 22:40:55, Søren Gjesse wrote:
I think these blcond methods should be moved to macro-assembler-mips.*
as it is
not MIPS instructions. I suggest the name Branch.
http://codereview.chromium.org/543161/diff/4001/5048#newcode562 src/mips/assembler-mips.h:562: void push(Register src) { Moved to macro-assembler. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Move to macro-assembler as Push.
http://codereview.chromium.org/543161/diff/4001/5048#newcode567 src/mips/assembler-mips.h:567: void push(Register src, Condition cond, Register tst1, Register tst2) { Moved to macro-assembler. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Ditto.
http://codereview.chromium.org/543161/diff/4001/5048#newcode579 src/mips/assembler-mips.h:579: void pop(Register dst) { Moved to macro-assembler. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Ditto.
http://codereview.chromium.org/543161/diff/4001/5048#newcode583 src/mips/assembler-mips.h:583: void pop() { Moved to macro-assembler. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Ditto.
http://codereview.chromium.org/543161/diff/4001/5048#newcode598 src/mips/assembler-mips.h:598: void jmp(Label* L) { Moved to macro-assembler. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Ditto.
http://codereview.chromium.org/543161/diff/4001/5048#newcode701 src/mips/assembler-mips.h:701: // Anyway we could surely implement this differently. Renamed to GenInstrRegister, GenInstrImmediate, GenInstrImm26. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Consider using the names Immediate, Jump and Register instead of 1, 2
and 3. http://codereview.chromium.org/543161/diff/4001/5046 File src/mips/constants-mips.h (right): http://codereview.chromium.org/543161/diff/4001/5046#newcode94 src/mips/constants-mips.h:94: class CRegisters { I don't know why it's still there. I rename it. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Please rename CRegisters to FPURegisters
http://codereview.chromium.org/543161/diff/4001/5046#newcode130 src/mips/constants-mips.h:130: static const int opcode_o = 26; Renamed and modified. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Please use the kXxxYyy naming of constants, e.g.
opcode_o -> kOpcodeShift opcode_l -> kOpcodeBits
and change the type from int to uint32_t.
static const uint32_t kOpcodeBits = 6; static const uint32_t kOpcodeShift = 21;
Also move enum constants below here. e.g.
static const uint32_t kOpcodeMask = ((1 << kOpcodeBits) - 1) <<
kOpcodeShift;
Also you might calculate each shift value by adding the bits of the
previous
field.
http://codereview.chromium.org/543161/diff/4001/5046#newcode277 src/mips/constants-mips.h:277: enum { Done. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Why are these constants in an enum? How about moving them up to the
constants
above?
http://codereview.chromium.org/543161/diff/4001/5046#newcode296 src/mips/constants-mips.h:296: // the 'U' prefix is used to specify unsigned comparisons. I don't like the confusion in the Intel naming. Also these condition are often used so the short U prefix is quite useful... Anyway if needed I can change it. On 2010/01/26 22:40:55, Søren Gjesse wrote:
I think the U prefix should be unsigned_ instead. As an alternative
you could
consider he Intel naming, where less and greater are unsigned and
above and
below are signed - even though that can also cause confusion.
http://codereview.chromium.org/543161/diff/4001/5046#newcode334 src/mips/constants-mips.h:334: enum C_Condition { Yes. This fits better with the new names. On 2010/01/26 22:40:55, Søren Gjesse wrote:
I suggest renaming C_Condition to FPUCondition.
http://codereview.chromium.org/543161/diff/4001/5046#newcode355 src/mips/constants-mips.h:355: // bool InstructionSetsConditionCodes(byte_* ptr) { Removed. I don't know what it was still doing here. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Please update this code sample - the HasS method is not part of this
class.
Maybe just remove it.
http://codereview.chromium.org/543161/diff/4001/5046#newcode400 src/mips/constants-mips.h:400: inline int rsField() const { I rename fields accessors for more coherence. I also add a few asserts to control fields access. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Rename rsField -> RsField - and for the other methods below.
Maybe ASSERT the instruction type is what is expected. e.g. for this
method
ASSERT instruction is immediate or register.
http://codereview.chromium.org/543161/diff/4001/5046#newcode469 src/mips/constants-mips.h:469: inline int32_t Imm16Field() const { Well I like the Imm16 because it says how many bits it uses, but I don't care too much it's used very locally. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Maybe rename Imm16Field -> ImmediateField?
Maybe ASSERT type is ImmediateInstructionType.
http://codereview.chromium.org/543161/diff/4001/5046#newcode473 src/mips/constants-mips.h:473: inline int32_t Imm26Field() const { Here I like it to be similar to Imm16Field instead of a long name. But again I don't care too much. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Maybe rename Imm26Field to InstructionIndexField.
Maybe ASSERT type is JumpInstructionType.
http://codereview.chromium.org/543161/diff/4001/5046#newcode484 src/mips/constants-mips.h:484: // Get the encoding type of the instruction. Yes good idea I do that. On 2010/01/26 22:40:55, Søren Gjesse wrote:
How about changing the type into an enum with the values
kImmediateType kJumpType kRegisterType
or some other names which provide more info than 1,2 and 3?
http://codereview.chromium.org/543161/diff/4001/5046#newcode506 src/mips/constants-mips.h:506: Renamed. On 2010/01/26 22:40:55, Søren Gjesse wrote:
Name constants like kXxxYyy.
http://codereview.chromium.org/543161/diff/4001/5034 File src/mips/disasm-mips.cc (right): http://codereview.chromium.org/543161/diff/4001/5034#newcode560 src/mips/disasm-mips.cc:560: // case TGEI: Removed this commented code. http://codereview.chromium.org/543161 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
