Use native shift instructions on Constantinople by axic · Pull Request #3797 · argotorg/solidity

axic · 2018-03-29T11:02:09Z

No description provided.

axic · 2018-03-29T11:02:32Z

@pirapira this will help in testing cpp-ethereum 😉

axic · 2018-03-29T12:06:47Z

Before merging this we need to adjust test to run on constantinople: https://github.com/ethereum/solidity/blob/develop/scripts/tests.sh#L106

axic · 2018-03-30T12:36:00Z

@chriseth do you have instructions somewhere how do you compile the eth binaries used in testing?

chriseth · 2018-04-03T17:09:46Z

@axic no special instructions for building... But yeah, we should perhaps automate that.

axic · 2018-04-05T09:54:30Z

Ran this with a local cpp-ethereum copy and weirdly enough it seems to be passing :)

chriseth · 2018-04-05T14:42:03Z

Conflict in the changelog. Also you might want to do a similar change in ABIFunctions.cpp

axic · 2018-04-05T14:45:48Z

In ABIFunctions there is shiftLeftFunction and shiftRightFunction and also 3 other places use mul(<x>, 2)

axic · 2018-04-05T15:02:55Z

libsolidity/codegen/ABIFunctions.cpp

+				}
+				)")
+				("functionName", functionName)
+				("shiftOp", _signed ? "sar" : "shl")


Typo: shl -> shr

axic · 2018-04-05T16:05:39Z

libsolidity/codegen/ABIFunctions.cpp

-			.render();
+		Whiskers templ(R"(
+		function <functionName>(value) -> newValue {
+			newValue := <shiftOp>(value, <secondArg>)


Actually I think this is broken because the top of the stack is secondArg for shifts, which in inline assembly would be shl(<secondArg>, value)

axic · 2018-04-05T16:25:59Z

@chriseth pushed the old version, but with the correct operand order

axic · 2018-04-05T16:27:26Z

libsolidity/codegen/ExpressionCompiler.cpp

 	case Token::SHL:
-		m_context << Instruction::SWAP1 << u256(2) << Instruction::EXP << Instruction::MUL;
+		if (m_context.evmVersion().hasBitwiseShifting())
+			m_context << Instruction::SHL;


This needs a SWAP1 then.

The swap is added above.

axic · 2018-04-06T13:01:26Z

The SAR test failures reminded me that SAR rounds differently to SDIV, it also shows that there is no test for this in ABIFunctions::shiftRightFunction because it didn't trigger a test failure 😉

chriseth · 2018-04-06T14:31:43Z

libsolidity/codegen/ExpressionCompiler.cpp

 		break;
 	case Token::SAR:
-		m_context << Instruction::SWAP1 << u256(2) << Instruction::EXP << Instruction::SWAP1 << (c_valueSigned ? Instruction::SDIV : Instruction::DIV);
+		// NOTE: SAR rounds differently than SDIV


And because of that, this would be a breaking change, wouldn't it?

Yes. We could think about having that as a breaking change though to fix signed right shifts.

axic · 2018-04-09T13:54:45Z

libsolidity/codegen/ExpressionCompiler.cpp

 	case Token::SHL:
-		m_context << Instruction::SWAP1 << u256(2) << Instruction::EXP << Instruction::MUL;
+		if (m_context.evmVersion().hasBitwiseShifting())
+			m_context << Instruction::SWAP1 << Instruction::SHL;


I think this SWAP1 is invalid here, it is done outside the switch already.

axic · 2018-04-18T22:04:25Z

Changelog.md

 Features:
 * Code Generator: Initialize arrays without using ``msize()``.
 * Code Generator: More specialized and thus optimized implementation for ``x.push(...)``
+ * Code Generator: Use native shift instructions on target Constantinople.


Need to move this.

axic · 2018-04-23T14:07:41Z

For example this test fails "regular_functions_exclude_fallback - GasMeterTests".

Code:

                contract A {
                        uint public x;
                        function() { x = 2; }
                }

Byzantium mode:

construction:
   87 + 35000 = 35087
external:
   fallback:	20123
   x():	394

EVM assembly:
    /* "9613.sol":16:131  contract A {... */
  mstore(0x40, 0x80)
  callvalue
    /* "--CODEGEN--":8:17   */
  dup1
    /* "--CODEGEN--":5:7   */
  iszero
  tag_1
  jumpi
    /* "--CODEGEN--":30:31   */
  0x0
    /* "--CODEGEN--":27:28   */
  dup1
    /* "--CODEGEN--":20:32   */
  revert
    /* "--CODEGEN--":5:7   */
tag_1:
    /* "9613.sol":16:131  contract A {... */
  pop
  dataSize(sub_0)
  dup1
  dataOffset(sub_0)
  0x0
  codecopy
  0x0
  return
stop

sub_0: assembly {
        /* "9613.sol":16:131  contract A {... */
      mstore(0x40, 0x80)
      jumpi(tag_1, lt(calldatasize, 0x4))
      calldataload(0x0)
      0x100000000000000000000000000000000000000000000000000000000
      swap1
      div
      0xffffffff
      and
      dup1
      0xc55699c
      eq
      tag_2
      jumpi
    tag_1:
...

Constantinople

construction:
   81 + 29200 = 29281
external:
   fallback:	20118
   x():	20118

EVM assembly:
    /* "9613.sol":16:131  contract A {... */
  mstore(0x40, 0x80)
  callvalue
    /* "--CODEGEN--":8:17   */
  dup1
    /* "--CODEGEN--":5:7   */
  iszero
  tag_1
  jumpi
    /* "--CODEGEN--":30:31   */
  0x0
    /* "--CODEGEN--":27:28   */
  dup1
    /* "--CODEGEN--":20:32   */
  revert
    /* "--CODEGEN--":5:7   */
tag_1:
    /* "9613.sol":16:131  contract A {... */
  pop
  dataSize(sub_0)
  dup1
  dataOffset(sub_0)
  0x0
  codecopy
  0x0
  return
stop

sub_0: assembly {
        /* "9613.sol":16:131  contract A {... */
      mstore(0x40, 0x80)
      jumpi(tag_1, lt(calldatasize, 0x4))
      and(0xffffffff, shr(0xe0, calldataload(0x0)))
      dup1
      0xc55699c
      eq
      tag_2
      jumpi
    tag_1:
...

I am not sure what is causing the 6000 higher gas cost on deployment, given the constant optimiser i not turned on, and the byzantium version has a large number with a lot zeroes for which no gas should be charged.

chriseth · 2018-04-23T14:33:55Z

zeros are only free in transaction data, but not for code deployment.

axic · 2018-04-30T21:16:49Z

@chriseth this seems to be working now

axic requested a review from chriseth March 29, 2018 11:02

axic mentioned this pull request Apr 3, 2018

Add simplification rule for bitwise shifting #3580

Merged

axic mentioned this pull request Apr 3, 2018

Document or automate (script?) the building of the eth binary #3813

Closed

chriseth previously approved these changes Apr 5, 2018

View reviewed changes

axic dismissed chriseth’s stale review via b931931 April 5, 2018 15:01

axic force-pushed the shift-constantinople branch from 0f6e529 to b931931 Compare April 5, 2018 15:01

axic commented Apr 5, 2018

View reviewed changes

axic force-pushed the shift-constantinople branch 2 times, most recently from f6d2f9c to 69ce190 Compare April 5, 2018 15:12

chriseth force-pushed the shift-constantinople branch from 69ce190 to 9794c50 Compare April 5, 2018 15:44

axic commented Apr 5, 2018

View reviewed changes

axic force-pushed the shift-constantinople branch from 9794c50 to 9a6a94e Compare April 5, 2018 16:25

axic commented Apr 5, 2018

View reviewed changes

axic force-pushed the shift-constantinople branch 4 times, most recently from 2ea8c65 to b5e879e Compare April 6, 2018 11:41

chriseth reviewed Apr 6, 2018

View reviewed changes

axic commented Apr 9, 2018

View reviewed changes

axic force-pushed the shift-constantinople branch 2 times, most recently from 84afba5 to ac9f667 Compare April 11, 2018 14:41

axic force-pushed the shift-constantinople branch from ac9f667 to 49c0ae1 Compare April 11, 2018 15:41

axic force-pushed the shift-constantinople branch from 49c0ae1 to 599c2b2 Compare April 18, 2018 22:03

axic commented Apr 18, 2018

View reviewed changes

axic force-pushed the shift-constantinople branch from 599c2b2 to 1c79613 Compare April 23, 2018 01:11

axic and others added 6 commits April 30, 2018 20:34

Use native shift instructions on Constantinople

22bfd3d

Use native shift instructions in ABIFunctions on Constantinople

c3608ea

Do not use SAR instead of SDIV in shifts because it rounds differently

52c9441

Removed signed shift right from the utilities.

2968639

Support shifts in gas estimator

059e232

Update source location test expectations

971941b

axic force-pushed the shift-constantinople branch from 1c79613 to 971941b Compare April 30, 2018 20:23

chriseth approved these changes May 2, 2018

View reviewed changes

chriseth merged commit a856135 into develop May 2, 2018

axic deleted the shift-constantinople branch May 3, 2018 22:32

Conversation

axic commented Mar 29, 2018

Uh oh!

axic commented Mar 29, 2018

Uh oh!

axic commented Mar 29, 2018

Uh oh!

axic commented Mar 30, 2018

Uh oh!

chriseth commented Apr 3, 2018

Uh oh!

axic commented Apr 5, 2018

Uh oh!

chriseth commented Apr 5, 2018

Uh oh!

axic commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Apr 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Apr 23, 2018

Uh oh!

chriseth commented Apr 23, 2018

Uh oh!

axic commented Apr 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments