In general, I like the approach, but I have a few comments.
Thanks,
Jacob
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.cc
File src/arm64/simulator-arm64.cc (right):
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.cc#newcode888
src/arm64/simulator-arm64.cc:888: public:
Is 'public' necessary here?
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.cc#newcode895
src/arm64/simulator-arm64.cc:895: typedef typename
make_unsigned<T>::type unsignedT;
I don't like this mechanism I'm afraid; it's not a common idiom (as far
as I know) and it's pretty confusing at first. If you can't replace it
with something cleaner, at least put the main declaration in the .cc
file with the specialisations and then add a very clear comment
explaining what's going on.
Another option would be to put it in the global 'utils.h'.
In this case, can't you just operate on uint64_t values and then cast to
<T> at the end? (I presume that the purpose is to get ShiftOperand to
return the proper type, instead of just int64_t.)
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.cc#newcode1306
src/arm64/simulator-arm64.cc:1306: instr->ImmDPShift());
This would be clearer (and fit on one line) if it declared shift_type
and shift_amount, like in VisitLogicalShifted.
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.cc#newcode1359
src/arm64/simulator-arm64.cc:1359: op2 &= kWRegMask;
Doesn't the new ShiftOperand apply that mask? If so, that would be a
good assertion.
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.cc#newcode1934
src/arm64/simulator-arm64.cc:1934: const T kMinimumInteger =
static_cast<T>(1) << (sizeof(T) * 8 - 1);
Consider adding a helper function to do this (perhaps in utils.h):
template <typename T> T MinInt();
template <> int64_t MinInt<int64_t>() { return INT64_MIN; }
template <> int32_t MinInt<int32_t>() { return INT32_MIN; }
We do that for FPDefaultNaN for example, for doubles and floats.
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.h
File src/arm64/simulator-arm64.h (right):
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.h#newcode152
src/arm64/simulator-arm64.h:152: value_ = new_value;
You probably need to set the value just once, to avoid temporarily
writing a sign-extended value into 'value_'. (This kind of thing has
upset the sampling profiler in the past as registers occasionally appear
to have unexpected values.)
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.h#newcode156
src/arm64/simulator-arm64.h:156: }
else {
ASSERT(size == (sizeof(value_) * kByteSize));
}
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.h#newcode160
src/arm64/simulator-arm64.h:160: void Set(T new_value) {
On 2014/05/16 11:15:47, Sven Panne wrote:
This method and the corresponding templatized Get is temporary only,
right? Or
is it the other way round? I don't fully understand the direction
we're going
here...
Indeed; I'd like to see what you're aiming for. I wouldn't expect this
to be temporary though; this method allows a constant size for memcpy,
so it's probably the best way to implement a simple 'put value X in a
register' case.
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.h#newcode170
src/arm64/simulator-arm64.h:170: if (size == kSRegSizeInBits) {
Use of SRegSize is surprising here; use WRegSize instead (as in the
other 'Set').
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.h#newcode184
src/arm64/simulator-arm64.h:184: int64_t value_;
Have you thought about how NEON Q registers might be represented? We
discussed it earlier in this issue but I don't think there was a
conclusion.
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.h#newcode187
src/arm64/simulator-arm64.h:187: STATIC_ASSERT(kXRegSize == kDRegSize);
It'd be better to put this next to the declaration of value_, but using
sizeof:
STATIC_ASSERT(kXRegSize == sizeof(value_));
STATIC_ASSERT(kDRegSize == sizeof(value_));
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.h#newcode356
src/arm64/simulator-arm64.h:356: bool Reg31ZeroMode(unsigned code,
Reg31Mode r31mode) const {
`IsZeroRegister(...)` would be a better name.
https://codereview.chromium.org/213943002/
--
--
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.
