Thanks for the feedback.
Direction:
I am replacing reg(reg_size, ... functions with either reg<int32/64_t> or
wreg,
xreg access. Getting rid of reg_size will allow for memcpy calls to have a
constant length, which should be optimized out.
class SimRegisterBase should have the Set/Get (unsigned size) member
functions
removed when this is all done.
CL:
There are less occurrences of the reg(reg_size, ... idiom than I originally
thought. Locally I am trying to keep the changes spread across git
commits, but
I'll squash them into this cl for review.
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:
On 2014/05/16 14:05:50, jbramley wrote:
Is 'public' necessary here?
Yes. Below it is accessed make_unsigned<T>::type
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;
On 2014/05/16 14:05:50, jbramley wrote:
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.)
I haven't found away around this yet.
Propagating a int32_t to a uint64_t sign extends it first.
int32_t value = 0x80000000;
int32_t result32 = static_cast<uint64_t>(value) >> 16; // = 0xffff8000
int32_t result64 = static_cast<uint32_t>(value) >> 16; // = 0x00008000
Register read/write access is signed at the moment. I could templatize
read_wreg<uint32_t> or do read_wreg_unsigned. But I was trying to be
simpler than that.
I was trying to do a drop in replacement without changing the algorithm.
std::make_unsigned is present in c++11
(http://www.cplusplus.com/reference/type_traits/make_unsigned/)
So the idea isn't mine. I was just trying to backport it. And the
implementation isn't mine either, it's cribbed together mostly from here
(http://stackoverflow.com/questions/16461561/c-conversion-from-t-to-unsigned-t)
I will look for a better alternative.
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.cc#newcode1306
src/arm64/simulator-arm64.cc:1306: instr->ImmDPShift());
On 2014/05/16 14:05:50, jbramley wrote:
This would be clearer (and fit on one line) if it declared shift_type
and
shift_amount, like in VisitLogicalShifted.
Done.
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.cc#newcode1359
src/arm64/simulator-arm64.cc:1359: op2 &= kWRegMask;
On 2014/05/16 14:05:50, jbramley wrote:
Doesn't the new ShiftOperand apply that mask? If so, that would be a
good
assertion.
ShiftOperand was returning a signed int64_t. It's returned as a int32_t
and propagated to a int64_t.
This has been an iterative process of trying to replace one function at
a time. Looking at this no, ShiftOperand can probably return an
unsigned. LogicalHelper writes the register and needs to be templatized
so that it isn't writing using reg_size.
This is what I was referring to in regards to transitory functions.
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);
On 2014/05/16 14:05:50, jbramley wrote:
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.
Thanks, great idea.
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#newcode160
src/arm64/simulator-arm64.h:160: void Set(T new_value) {
On 2014/05/16 14:05:50, jbramley wrote:
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.
My direction is to only use the templatized Get/Set. This results in a
memcpy with a constant length.
https://codereview.chromium.org/213943002/diff/80001/src/arm64/simulator-arm64.h#newcode184
src/arm64/simulator-arm64.h:184: int64_t value_;
On 2014/05/16 14:05:50, jbramley wrote:
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.
I don't have a fully formed solution yet. I do realize that it will be
necessary to land this cl.
One thing I have considered is declaring all registers as 128 bit. All
access to registers go through this class, there are no native
operations on the backing store. So it will waste space. 64 registers
* extra 8 bytes. And it doesn't exactly follow the hw model.
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 {
On 2014/05/16 14:05:50, jbramley wrote:
`IsZeroRegister(...)` would be a better name.
Done.
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.
