All the comments in test-assembler-mips64.cc likely also apply to
test-assembler-mips.cc also. Sorry, I was somewhat in-accurate in my
previous
review comments on that file, and I may have misled you a bit about the
runtime
vs generation time issues. I think it is clear now.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc
File test/cctest/test-assembler-mips64.cc (right):
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4450
test/cctest/test-assembler-mips64.cc:4450: uint64_t run_align(uint64_t
rs, uint64_t rt, uint8_t bp) {
I explain in more detail below -- but I don't like these name rs, rt
here, since you are not using them to specify registers in the generated
code, but rather just values that are passed thru the function-call ABI
(in regs a0, a1).
bp is used for code-generation, and is named fine, but should not be in
the runtime call.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4468
test/cctest/test-assembler-mips64.cc:4468: uint64_t rd =
Also explained in more detail below, this is not "really" rd. It the
result being returned from a function, which per ABI is register v0 --
which you've hardcoded above into the instruction.
Please more complete explanation in code for run_lwupc(), and adjust
this function and all the others with this expectation.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4487
test/cctest/test-assembler-mips64.cc:4487: // rd_expected,
rs, rt, bp
As below, object to these (register) names here for what is
expected-result, and input data.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4633
test/cctest/test-assembler-mips64.cc:4633: uint64_t rs =
nit: I'm not comfortable with this use of 'rs' here - I think it
confuses the use of the 'rs' register field of the instruction, with the
returned 'result' of the assembly code fragment above. By the mips ABI,
the result of a function call is returned in register v0, regardless of
what instruction you use above or what the value of the 'rs' field in a
particular instruction is. Better to 'result' or something like that.
Next function, too.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4647
test/cctest/test-assembler-mips64.cc:4647: // As rs register will be
used v0 register.
nit: I also think 'imm16' is not an optimal name for this field.
Something like 'offset' or 'pc_offset' is more descriptive of what you
are doing, rather the repeating the name of the field within the raw
instruction. (this applies to several other of these functions as well.
I think both lines of this comment should be removed (or at least
re-worded) once the naming changes are done.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4722
test/cctest/test-assembler-mips64.cc:4722: // rs - A word from memory
will be stored in t8 register.
I think this comment no longer applies, and should be deleted.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4724
test/cctest/test-assembler-mips64.cc:4724: uint64_t expected_rs;
nit: I think this should be named expected_res, or expected_result. It
seems slightly confusing with use of 'rs' as a register number of an
intstruction.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4748
test/cctest/test-assembler-mips64.cc:4748: uint64_t run_lwupc(int rs,
int offset) {
I made an incorrect statement in patchset 3 review of
test-assembler-mips.cc TEST(r6_lwpc) -- about code-generation time vs
runtime. I was partly correct, let me try to explain more clearly what I
was trying to say there, in the context of this function, and its
caller.
This function takes 'rs' as a parameter from the caller. It absolutely
CAN use that 'rs' value to generate the assembly code. However, you do
not do that here, since you hard-code the register: __ lwupc(t8,
offset);
That could be written: __ lwupc(rs, offset); and it would be correct.
(However, from the caller: TEST(r6_lwupc), you never vary that register,
it is always t8.code(), so there is no point to having the parameter. If
you did change to varying registers, you would also have use __ mov(v0,
rs); . That would also be fine. Also, note that you are _generating_ the
instruction with an encoded value of 'offset', which again is fine.
What looks very wrong to me is the CALL_GENERATED_CODE(f, rs, offset,
...) where you pass in rs, and offset at the runtime. Now you don't use
these at runtime, but you should not pass them in, because it is
confusing to the reader.
Please take a look at all your test functions, to be sure there is not
confusion between generation-time, and run-time.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4807
test/cctest/test-assembler-mips64.cc:4807: { t8.code(), -4,
0x250c0003 },
As mentioned above, if you're going to pass in a register number to use,
you might as well vary it from call to call -- just for fun.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4873
test/cctest/test-assembler-mips64.cc:4873:
reinterpret_cast<uint64_t>(CALL_GENERATED_CODE(f, offset, 0, 0, 0, 0));
As above: the value of offset is used at generation time, and cannot
(and is not) used when the function is called. Please remove it here.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4885
test/cctest/test-assembler-mips64.cc:4885: // the program counter for
the jic instruction.
nit: this comment is not needed. The code above is clear that the input
to jic instr is t0.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4906
test/cctest/test-assembler-mips64.cc:4906: uint64_t run_beqzc(int32_t
rs, int32_t offset) {
The first parameter here is used as a 'value' that you pass to the
function thru ABI first parameter register, which is a0, and is
hard-coded in the beqzc instruction. You are not really passing in a
'rs' field, so I suggest not calling it that.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4945
test/cctest/test-assembler-mips64.cc:4945:
reinterpret_cast<uint64_t>(CALL_GENERATED_CODE(f, rs, offset, 0, 0, 0));
You are passing in a 'value' as first param, please rename that param.
You cannot pass offset at runtime, so please make the 2nd param 0.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode4962
test/cctest/test-assembler-mips64.cc:4962: // rs_value, offset,
expected_res
I suggest you just call this value - since you have hard-coded the 'rs'
field of the instruction to a0, this has nothing really to do with 'rs'.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode5105
test/cctest/test-assembler-mips64.cc:5105: // As rs register will be
used v0 register.
nit: please delete this comment. I'm not sure what the first line is
trying to say, and the 2nd line about v0 is hard-coded above, and is
clear.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode5146
test/cctest/test-assembler-mips64.cc:5146: __ mov(v0, t8);
As above: If you are passing in rs, I suggest you use it here, rather
than hard-code t8.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode5169
test/cctest/test-assembler-mips64.cc:5169:
reinterpret_cast<uint64_t>(CALL_GENERATED_CODE(f, rs, offset, 0, 0, 0));
As above, you cannot use rs or offset at runtime, just pass 0.
https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assembler-mips64.cc#newcode5272
test/cctest/test-assembler-mips64.cc:5272: { 0, (abs(-100) -
10 + 1 + 99) },
nit; weird brace alignment, if you can line them up, great, if
clang-format prevents, then do the entire table without the extra
spaces.
https://codereview.chromium.org/1144373003/
--
--
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.
