new branch for PR, attempted vcc fix, other cleanup #4233

gcolvin · 2017-07-07T04:34:54Z

The specification is here - ethereum/EIPs#616
The previous PR and review is here - /pull/4201

@gumb0 This is the latest code for review.
@pirapira SIMD vectors and operations are typed. I would be very interested in what you have to say about how well the correct use of the types can be validated and otherwise worked with formally.

codecov-io · 2017-07-07T05:35:34Z

Codecov Report

Merging #4233 into develop will increase coverage by 0.08%.
The diff coverage is 80%.

@@             Coverage Diff             @@
##           develop    #4233      +/-   ##
===========================================
+ Coverage    67.27%   67.36%   +0.08%     
===========================================
  Files          302      306       +4     
  Lines        23340    23435      +95     
===========================================
+ Hits         15702    15787      +85     
- Misses        7638     7648      +10

Impacted Files	Coverage Δ
test/tools/fuzzTesting/fuzzHelper.cpp	`43.75% <ø> (-10.55%)`	⬇️
libevmcore/Instruction.cpp	`31.81% <ø> (ø)`	⬆️
libevm/VMOpt.cpp	`91.54% <ø> (ø)`	⬆️
libevm/VM.h	`76.92% <ø> (ø)`	⬆️
libevmcore/Instruction.h	`100% <ø> (ø)`	⬆️
libevm/VM.cpp	`95.79% <80%> (-0.16%)`	⬇️
libethereum/Account.h	`95.45% <0%> (-4.55%)`	⬇️
libp2p/Host.cpp	`72.36% <0%> (-1.06%)`	⬇️
libp2p/Common.cpp	`52% <0%> (-0.8%)`	⬇️
... and 40 more

gumb0 · 2017-07-07T12:30:02Z

libevm/VM.cpp

+			updateIOGas();
+			*m_RP++ = m_PC++;
+			m_PC = decodeJumpDest(m_code.data(), m_PC);
+			}


too many braces

Weird. This compiled.

No - this is EIP_615 code, which I haven't compiled for a while.

Now it compiles. Thanks.

gumb0 · 2017-07-07T12:30:29Z

libevm/VM.cpp

+
+			uint8_t b = ++m_PC;
+			uint8_t c = ++m_PC;
+			xput(m_code[b], m_code[c]); ++m_PC;


each statement on separate line

gumb0 · 2017-07-07T12:30:35Z

libevm/VM.cpp

+
+			uint8_t b = ++m_PC;
+			uint8_t c = ++m_PC;
+			xget(m_code[b], m_code[c]); ++m_PC;


each statement on separate line

gumb0 · 2017-07-07T12:30:57Z

libevm/VM.cpp

@@ -1096,7 +1491,7 @@ void VM::interpretCases()
 #if EVM_HACK_DUP_64
 			*(uint64_t*)m_SPP = *(uint64_t*)(m_SP + n);
 #else
-			m_SPP[0] = m_SP[n];
+			new(m_SPP) u256(m_SP[n]);


What is this change for?

The stack slot being copied into may no longer hold a u256, so we construct a new one in the memory, rather than assign. I added a comment.

gumb0 · 2017-07-07T12:32:10Z

libevm/VM.h

+	void xswizzle(uint8_t);
+	void xshuffle(uint8_t);
+
+	u256 vtow(uint8_t _b, const u256& _in);


u256 const&

gumb0 · 2017-07-07T12:33:52Z

libevm/VM.h

+	void xshuffle(uint8_t);
+
+	u256 vtow(uint8_t _b, const u256& _in);
+	void wtov(uint8_t _b, u256 _in, u256& _o_out);


Make this return u256, since you've made it for vtow.
Pass _in by const reference

CHeck comments on implementation. Arguments and returns for this and vtow() are as they need to be/

Ok, the convention for output params is o_out, without leading underscore

Ok. But I wonder why the inconsistency? Either all arguments should have a leading underscore or they shouldn't.

gumb0 · 2017-07-07T12:43:01Z

libevm/VMConfig.h

 //
 // EVM_TRACE              - provides various levels of tracing

+#ifndef EIP_615
+	#define EIP_615 false


I would prefer defining these in .cmake files instead.
Then we'd be able to build with or without it just by changing cmake command line.

Or maybe it's not even necessary to modify .cmake files. maybe it would be enough just to delete these #defines, then try to build with cmake .. -DEIP_615=true ?

Presumably these will stay off in production until EIPs are accepted, and then be removed. So in the meantime it's good to be able to switch configurations with minimal recompilation. That's my main consideration. cmake files might work. Rebuilding with cmake at the top would not.

Looking at the code again, I still like this pattern. It allows for external configuration (via cmake or whatever) - if a macro has a definition when compiled it is respected, and if it doesn't an appropriate default is chosen. During development I just change the defaults to quickly switch configurations.

gumb0 · 2017-07-07T12:57:18Z

libevm/VMSIMD.cpp

+#define AND( x1, x2) ((x1) & (x2))
+#define OR(  x1, x2) ((x1) | (x2))
+#define XOR( x1, x2) ((x1) ^ (x2))
+#define NOT( x1, x2) (~x1)


should be (~(x1))

gumb0 · 2017-07-07T12:57:48Z

libevm/VMSIMD.cpp

+#define NOT( x1, x2) (~x1)
+#define SHR( x1, x2) ((x1) >> (x2))
+#define SHL( x1, x2) ((x1) << (x2))
+#define ROL( x1, x2) (((x1) << x2)|((x1) >> (sizeof(x1) * 8 - (x2))))


should be (((x1) << (x2))|...

gumb0 · 2017-07-07T12:58:00Z

libevm/VMSIMD.cpp

+#define SHR( x1, x2) ((x1) >> (x2))
+#define SHL( x1, x2) ((x1) << (x2))
+#define ROL( x1, x2) (((x1) << x2)|((x1) >> (sizeof(x1) * 8 - (x2))))
+#define ROR( x1, x2) (((x1) >> x2)|((x1) << (sizeof(x1) * 8 - (x2))))


should be (((x1) << (x2))|...

I regret I couldn't make these work as templates and avoid all these parentheses.

gumb0 · 2017-07-07T12:58:45Z

libevm/VMSIMD.cpp

+
+inline uint8_t pow2N(uint8_t n)
+{
+	static uint8_t exp[6] = { 1, 2, 4, 8, 16, 32 };


maybe add assert(n < 6)

Do we ever turn asserts on? Where? There are a lot of them that could be added to double-check everything for this EIP and 615 that is supposed to be validated.

They are on in debug build. I build debug all the time

OK. RelDbg is usually enough for me. I'll put more in for the next PR.

gumb0 · 2017-07-07T12:59:00Z

libevm/VMSIMD.cpp

+void VM::xrol (uint8_t _b) { EVALXOPU(ROL, _b); }
+void VM::xror (uint8_t _b) { EVALXOPU(ROR, _b); }
+
+inline uint8_t pow2N(uint8_t n)


should be _n

gumb0 · 2017-07-07T13:07:32Z

libevm/VMSIMD.cpp

+		for (int j = n,  i = n - 1; 0 <= i; --i)
+		{
+			int v = 0;
+			v = v8x32(m_SPP[0])[i];


I guess this should be v = v16x16(m_SPP[0])[i]; ?

yep, thanks

gumb0 · 2017-07-07T13:08:01Z

libevm/VMSIMD.cpp

+		for (int j = n,  i = n - 1; 0 <= i; --i)
+		{
+			int v = 0;
+			v = v8x32(m_SPP[0])[i];


yep, thanks

gumb0 · 2017-07-07T13:08:11Z

libevm/VMSIMD.cpp

+		for (int j = n,  i = n - 1; 0 <= i; --i)
+		{
+			int v = 0;
+			v = v8x32(m_SPP[0])[i];


yep, thanks

gumb0 · 2017-07-07T13:27:23Z

libevm/VMSIMD.cpp

+			break;
+		case 1:
+			for (int i = 0; i < m; ++i)
+				v16x16(m_SPP[0])[i] = v8x32(m_SP[0])[v16x16(m_SP[1])[i] % 16];


Hm, not sure if this is correct, shouldn't source vector and result vector have the same type (which is t) ?
That is, I think left-hand side here should be v8x32(m_SPP[0])[i]

Spec says they can be different. We could take up in the EIP issue whether that is a good idea.

Hm, ok, but the code contains only two bytes after XGET opcode, for the type of source vector and the type of get vector (indices vector).
Then this implementation considers result type to be equal get vector type, I find it weird.

For the most general implementation we probably should have 3 bytes following XGET opcode, for types of all 3 vectors?
I'll add a comment to EIP

I see now, XGET really replaces each index in the stack word with a value from source vector, that's why index vector type and result vector type are the same.

gumb0 · 2017-07-07T13:28:10Z

libevm/VMSIMD.cpp

+		{
+		case 0:
+			for (int i = 0; i < m; ++i)
+				v8x32 (m_SPP[0])[i] = v16x16(m_SP[1])[v8x32 (m_SP[0])[i] % 32];


The same problem here, assigning uint16_t to uint8_t doesn't look correct.
Left-hand side should be v16x16 ?

Same answer as above.

To say a little more - XGET is the way to get elements out of a vector. Letting the destination have a different type than the source allows for casting, and I've allowed for both downcasting and upcasting. That's all.

gumb0 · 2017-07-10T11:39:22Z

libevm/VMSIMD.cpp

+			break;
+		case 1:
+			for (int i = 0; i < m; ++i)
+				v16x16(m_SPP[0])[v8x32(m_SP[1])[i] % 16] = v8x32(m_SP[0])[i];


And this implementation of XPUT considers source vector type to be the same as index vector type (which is t)
Why not consider source vector type be the same as result vector (which is u) ?

Here I'm still confused, it reads only two vectors from the stack, but XPUT is supposed to take 3 vectors?

As discussed at the EIP I need to clean up the definition, and then I can be sure I have implemented the definition. But this looks wrong, and your suggestion sound right, thanks.

gumb0 · 2017-07-10T12:08:03Z

libevm/VM.cpp

-			m_SPP[0] = m_SP[n];
+			// the stack slot being copied into may no longer hold a u256
+			// so we construct a new one in the memory, rather than assign
+			new(m_SPP) u256(m_SP[n]);


So do I understand this correctly, that this is done to avoid copy assignment operator interpreting the data as u256 and somehow corrupting it, since it can be not u256 at all?

But this placement new still calls the copy constructor, and copy constructor will still interpret it as u256.

Maybe we should memcpy or std:copy the bytes here? Which is against the rules since uint256 is not a POD...
But I don't see how placement new helps here

It's not the source that is the problem - it's a u256. It's the destination that's the problem - it might be a vector. The assignment operator would treat the destination as a u256, which might blow up. Placement new treats it as raw memory. (Then we are just down to the problem that the destination's destructor is not called. For our u256 that is not a problem, but I think we will need XPOP and XPUSH to fix that in general.)

But source can also be a vector? Or will validator not allow it?

Right - it's invalid.

Oh - we will need XDUP and XSWAP too. Either that or implementations will be forced to keep track of the type of every stack slot, and this routine will involve checking the type of the source and destination. In C++ we can fix that by making u256 a POD, but some other languages can't get that close to the metal.

gumb0 · 2017-07-11T10:54:39Z

libevm/VMSIMD.cpp

+		for (int i = 0; i < n; ++i)
+		{
+			int j = v8x32(m_SP[0]) [i];
+			v8x32 (m_SPP[0])[i] = j < n ? v8x32(m_SP[2]) [j] : v8x32 (m_SP[2])[j % n];


This doesn't access source 1 vector, should be j < n ? v8x32(m_SP[1]) [j] : v8x32 (m_SP[2])[j % n]; as I understand

I think so. thanks.

Yes, this is clearly wrong - the two sides of the condition should be different. But the spec is unclear as to which is which, and the example in the spec is also clearly wrong. So both need fixing. I notice in this spec and others it unclear whether the "first" argument is the first item pushed on the stack or item on the top of the stack - SP[n] versus SP[0].

gumb0 · 2017-07-11T14:41:50Z

libevm/VMValidate.cpp

-	byte OP;
-	for (PC = 0; (OP = m_code[PC]); ++PC)
-		if  (OP == byte(Instruction::BEGINSUB))
+	Instruction OP;


Are the changes in this file supposed to go into this PR ? They look not really related to SIMD

Changes for SIMD (and some previous ones) broke VMValidate, so I wanted it to compile again. Changes are pretty much small stuff like changed signatures and casts.

gumb0

Ok, I think I've finished reviewing it now. Implementations of xput and xshuffle still have errors to fix, after that we could merge.

gumb0 · 2017-07-13T08:30:30Z

libevm/VMSIMD.cpp

@@ -41,28 +41,30 @@ inline a32x8  const& v32x8 (u256 const& _stack_item) { return (a32x8&) *(a32x8*)
 inline a16x16 const& v16x16(u256 const& _stack_item) { return (a16x16&)*(a16x16*)&_stack_item; }
 inline a8x32  const& v8x32 (u256 const& _stack_item) { return (a8x32&) *(a8x32*) &_stack_item; }

+class Pow2Bits enum { BITS_8, BITS_16, BITS_32, BITS_64 };


Wait, this doesn't compile for me at all. Did you intend it to be enum class? Then BITS_8 etc. will be scoped (should be Pow2Bits::BITS_8 etc. everywhere) and implicit conversion between uint8_t and Pow2Bits won't work in switches.
Probably easier to use plain enum, although coding standards discourage it

Naming: all-uppercase is used only for macros, members should be named something like Bits8, Bits16 etc.

enum itself probably could be named LaneWidth

Thought I'd compiled it, but I did intend enum class, and I was pleasantly surprised not to see the problems you mentioned, so the config macro was probably turned off already. A simple enum is what I'm sure compiled, and it makes sense in the scope of just one file. I'll fix up the names.

gumb0 · 2017-07-13T08:32:40Z

libevm/VMSIMD.cpp

@@ -169,35 +171,35 @@ u256 VM::vtow(uint8_t _b, const u256& _in)
 }

 // out must be by reference because it is really just memory for a vector
-void VM::wtov(uint8_t _b, u256 _in, u256& _o_out)
+void VM::wtov(uint8_t _type, u256 _in, u256& _o_out)


Naming: o_out

gumb0 · 2017-07-13T08:33:25Z

libevm/VMSIMD.cpp

@@ -426,108 +426,107 @@ void VM::xpush(uint8_t _b)
 	}
 }

-void VM::xget(uint8_t _b, uint8_t _c)
+void VM::xget(uint8_t _src_type, uint8_t _idx_type)


Naming: don't use underscores other than for prefix, should be _srcType & _idxType

Thought I'd search and replaced all of these...

gumb0 · 2017-07-13T08:33:55Z

libevm/VMSIMD.cpp

@@ -538,107 +537,106 @@ void VM::xget(uint8_t _b, uint8_t _c)
 	}
 }

-void VM::xput(uint8_t _b, uint8_t _c)
+void VM::xput(uint8_t _src_type, uint8_t _dst_type)


Naming: _srcType & _dstType

Thought I'd search and destroyed all of these too...

Still - better than _b and _c.

A grep on _[a-z]+_[a-z]+ also smoked out some _o_out and _stack_item hiding in plain sight.

gcolvin · 2017-07-13T08:55:58Z

Travis is going to fail soon - Homebrew is broken. No merging until that is fixed.

gumb0 · 2017-07-14T10:14:18Z

libevm/VMSIMD.cpp

+	switch (width)
+	{
+	case Bits8:
+		for (int j = 1,  i = count - 1; 0 <= i; --i)


Is j initialization correct here and in other cases? it will go negative here if count >= 2

It was initializeed as j = n in earlier commits

I need to dig into this after the AllCoreDevs meeting. This looks like the remains of a merge conflict that I reconciled badly. It should be n, I think I agree, but n is gone.

gumb0 · 2017-07-14T10:15:41Z

libevm/VMSIMD.cpp

+	switch (width)
+	{
+	case Bits8:
+		for (int j = 1,  i = count - 1; 0 <= i; --i)


Here too, should it be j = count ?

gcolvin · 2017-07-15T06:25:04Z

Thanks, @gumb0 !

new branch for PR, attempted vcc fix, other cleanup

0240696

gcolvin added the in progress label Jul 7, 2017

gcolvin mentioned this pull request Jul 7, 2017

SIMD #4201

Closed

gumb0 reviewed Jul 7, 2017

View reviewed changes

gcolvin added 2 commits July 7, 2017 10:18

more review by Andrei

1e90277

everything compiles in all configs

09ea060

gumb0 reviewed Jul 10, 2017

View reviewed changes

gumb0 reviewed Jul 11, 2017

View reviewed changes

gumb0 suggested changes Jul 11, 2017

View reviewed changes

meaningful names, clean up arguments

68e4a3b

gumb0 reviewed Jul 13, 2017

View reviewed changes

name nits

4764f14

gumb0 reviewed Jul 14, 2017

View reviewed changes

more name nits

e7ca6e7

gumb0 approved these changes Jul 15, 2017

View reviewed changes

gcolvin merged commit 5206222 into ethereum:develop Jul 15, 2017

gcolvin deleted the symd branch July 15, 2017 06:24

gcolvin removed the in progress label Jul 18, 2017

new branch for PR, attempted vcc fix, other cleanup #4233

new branch for PR, attempted vcc fix, other cleanup #4233

Conversation

gcolvin commented Jul 7, 2017 • edited Loading

codecov-io commented Jul 7, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcolvin Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

gcolvin Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcolvin commented Jul 13, 2017

gumb0 Jul 14, 2017 • edited Loading

gcolvin commented Jul 7, 2017 •

edited

Loading

codecov-io commented Jul 7, 2017 •

edited

Loading

gcolvin Jul 7, 2017 •

edited

Loading

gumb0 Jul 10, 2017 •

edited

Loading

gcolvin Jul 10, 2017 •

edited

Loading

gumb0 Jul 14, 2017 •

edited

Loading