Merge pull request #13118 from ethereum/improvedCopyValueArrayToStorageFunction

Improved copy value-type-array to storage function

Author: ekpyron

Changelog.md

@@ -5,6 +5,8 @@ Language Features:
 
 Compiler Features:
  * TypeChecker: Support using library constants in initializers of other constants.
+ * Yul IR Code Generation: Improved copy routines for arrays with packed storage layout.
+
 
 Bugfixes:
  * Commandline Interface: Disallow the following options outside of the compiler mode: ``--via-ir``,``--metadata-literal``, ``--metadata-hash``, ``--model-checker-show-unproved``, ``--model-checker-div-mod-no-slacks``, ``--model-checker-engine``, ``--model-checker-invariants``, ``--model-checker-solvers``, ``--model-checker-timeout``, ``--model-checker-contracts``, ``--model-checker-targets``.
@@ -30,6 +32,7 @@ Compiler Features:
 Bugfixes:
  * ABI Encoder: When encoding an empty string coming from storage do not add a superfluous empty slot for data.
  * Common Subexpression Eliminator: Process assembly items in chunks with maximum size of 2000. It helps to avoid extremely time-consuming searches during code optimization.
+ * Yul IR Code Generation: More robust cleanup in corner cases during memory to storage copies.
  * Yul Optimizer: Do not remove ``returndatacopy`` in cases in which it might perform out-of-bounds reads that unconditionally revert as out-of-gas. Previously, any ``returndatacopy`` that wrote to memory that was never read from was removed without accounting for the out-of-bounds condition.
 
 

libsolidity/codegen/YulUtilFunctions.cpp

@@ -1797,8 +1797,11 @@ string YulUtilFunctions::copyArrayToStorageFunction(ArrayType const& _fromType,
 
 	if (_fromType.isByteArrayOrString())
 		return copyByteArrayToStorageFunction(_fromType, _toType);
-	if (_fromType.dataStoredIn(DataLocation::Storage) && _toType.baseType()->isValueType())
-		return copyValueArrayStorageToStorageFunction(_fromType, _toType);
+	if (_toType.baseType()->isValueType())
+		return copyValueArrayToStorageFunction(_fromType, _toType);
+
+	solAssert(_toType.storageStride() == 32);
+	solAssert(!_fromType.baseType()->isValueType());
 
 	string functionName = "copy_array_to_storage_from_" + _fromType.identifier() + "_to_" + _toType.identifier();
 	return m_functionCollector.createFunction(functionName, [&](){
@@ -1812,43 +1815,30 @@ string YulUtilFunctions::copyArrayToStorageFunction(ArrayType const& _fromType,
 				let srcPtr := <srcDataLocation>(value)
 
 				let elementSlot := <dstDataLocation>(slot)
-				let elementOffset := 0
 
 				for { let i := 0 } lt(i, length) {i := add(i, 1)} {
 					<?fromCalldata>
-						let <elementValues> :=
+						let <stackItems> :=
 						<?dynamicallyEncodedBase>
 							<accessCalldataTail>(value, srcPtr)
 						<!dynamicallyEncodedBase>
 							srcPtr
 						</dynamicallyEncodedBase>
-
-						<?isValueType>
-							<elementValues> := <readFromCalldataOrMemory>(<elementValues>)
-						</isValueType>
 					</fromCalldata>
 
 					<?fromMemory>
-						let <elementValues> := <readFromCalldataOrMemory>(srcPtr)
+						let <stackItems> := <readFromMemoryOrCalldata>(srcPtr)
 					</fromMemory>
 
 					<?fromStorage>
-						let <elementValues> := srcPtr
+						let <stackItems> := srcPtr
 					</fromStorage>
 
-					<updateStorageValue>(elementSlot, elementOffset, <elementValues>)
+					<updateStorageValue>(elementSlot, <stackItems>)
 
 					srcPtr := add(srcPtr, <srcStride>)
 
-					<?multipleItemsPerSlot>
-						elementOffset := add(elementOffset, <storageStride>)
-						if gt(elementOffset, sub(32, <storageStride>)) {
-							elementOffset := 0
-							elementSlot := add(elementSlot, 1)
-						}
-					<!multipleItemsPerSlot>
-						elementSlot := add(elementSlot, <storageSize>)
-					</multipleItemsPerSlot>
+					elementSlot := add(elementSlot, <storageSize>)
 				}
 			}
 		)");
@@ -1870,25 +1860,22 @@ string YulUtilFunctions::copyArrayToStorageFunction(ArrayType const& _fromType,
 		}
 		templ("resizeArray", resizeArrayFunction(_toType));
 		templ("arrayLength",arrayLengthFunction(_fromType));
-		templ("isValueType", _fromType.baseType()->isValueType());
 		templ("dstDataLocation", arrayDataAreaFunction(_toType));
 		if (fromMemory || (fromCalldata && _fromType.baseType()->isValueType()))
-			templ("readFromCalldataOrMemory", readFromMemoryOrCalldata(*_fromType.baseType(), fromCalldata));
-		templ("elementValues", suffixedVariableNameList(
-			"elementValue_",
+			templ("readFromMemoryOrCalldata", readFromMemoryOrCalldata(*_fromType.baseType(), fromCalldata));
+		templ("stackItems", suffixedVariableNameList(
+			"stackItem_",
 			0,
 			_fromType.baseType()->stackItems().size()
 		));
-		templ("updateStorageValue", updateStorageValueFunction(*_fromType.baseType(), *_toType.baseType()));
+		templ("updateStorageValue", updateStorageValueFunction(*_fromType.baseType(), *_toType.baseType(), 0));
 		templ("srcStride",
 			fromCalldata ?
 			to_string(_fromType.calldataStride()) :
 				fromMemory ?
 				to_string(_fromType.memoryStride()) :
 				formatNumber(_fromType.baseType()->storageSize())
 		);
-		templ("multipleItemsPerSlot", _toType.storageStride() <= 16);
-		templ("storageStride", to_string(_toType.storageStride()));
 		templ("storageSize", _toType.baseType()->storageSize().str());
 
 		return templ.render();
@@ -1973,16 +1960,14 @@ string YulUtilFunctions::copyByteArrayToStorageFunction(ArrayType const& _fromTy
 	});
 }
 
-
-string YulUtilFunctions::copyValueArrayStorageToStorageFunction(ArrayType const& _fromType, ArrayType const& _toType)
+string YulUtilFunctions::copyValueArrayToStorageFunction(ArrayType const& _fromType, ArrayType const& _toType)
 {
 	solAssert(_fromType.baseType()->isValueType(), "");
 	solAssert(_toType.baseType()->isValueType(), "");
 	solAssert(_fromType.baseType()->isImplicitlyConvertibleTo(*_toType.baseType()), "");
 
 	solAssert(!_fromType.isByteArrayOrString(), "");
 	solAssert(!_toType.isByteArrayOrString(), "");
-	solAssert(_fromType.dataStoredIn(DataLocation::Storage), "");
 	solAssert(_toType.dataStoredIn(DataLocation::Storage), "");
 
 	solAssert(_fromType.storageStride() <= _toType.storageStride(), "");
@@ -1991,42 +1976,51 @@ string YulUtilFunctions::copyValueArrayStorageToStorageFunction(ArrayType const&
 	string functionName = "copy_array_to_storage_from_" + _fromType.identifier() + "_to_" + _toType.identifier();
 	return m_functionCollector.createFunction(functionName, [&](){
 		Whiskers templ(R"(
-			function <functionName>(dst, src) {
+			function <functionName>(dst, src<?isFromDynamicCalldata>, len</isFromDynamicCalldata>) {
+				<?isFromStorage>
 				if eq(dst, src) { leave }
-				let length := <arrayLength>(src)
+				</isFromStorage>
+				let length := <arrayLength>(src<?isFromDynamicCalldata>, len</isFromDynamicCalldata>)
 				// Make sure array length is sane
 				if gt(length, 0xffffffffffffffff) { <panic>() }
 				<resizeArray>(dst, length)
 
-				let srcSlot := <srcDataLocation>(src)
+				let srcPtr := <srcDataLocation>(src)
 				let dstSlot := <dstDataLocation>(dst)
 
 				let fullSlots := div(length, <itemsPerSlot>)
 
-				let srcSlotValue := sload(srcSlot)
+				<?isFromStorage>
+				let srcSlotValue := sload(srcPtr)
 				let srcItemIndexInSlot := 0
+				</isFromStorage>
+
 				for { let i := 0 } lt(i, fullSlots) { i := add(i, 1) } {
 					let dstSlotValue := 0
-					<?sameType>
+					<?sameTypeFromStorage>
 						dstSlotValue := <maskFull>(srcSlotValue)
-						<updateSrcSlotValue>
-					<!sameType>
+						<updateSrcPtr>
+					<!sameTypeFromStorage>
 						<?multipleItemsPerSlotDst>for { let j := 0 } lt(j, <itemsPerSlot>) { j := add(j, 1) } </multipleItemsPerSlotDst>
 						{
-							let itemValue := <convert>(
+							<?isFromStorage>
+							let <stackItems> := <convert>(
 								<extractFromSlot>(srcSlotValue, mul(<srcStride>, srcItemIndexInSlot))
 							)
-							itemValue := <prepareStore>(itemValue)
+							<!isFromStorage>
+							let <stackItems> := <readFromMemoryOrCalldata>(srcPtr)
+							</isFromStorage>
+							let itemValue := <prepareStore>(<stackItems>)
 							dstSlotValue :=
 							<?multipleItemsPerSlotDst>
 								<updateByteSlice>(dstSlotValue, mul(<dstStride>, j), itemValue)
 							<!multipleItemsPerSlotDst>
 								itemValue
 							</multipleItemsPerSlotDst>
 
-							<updateSrcSlotValue>
+							<updateSrcPtr>
 						}
-					</sameType>
+					</sameTypeFromStorage>
 
 					sstore(add(dstSlot, i), dstSlotValue)
 				}
@@ -2035,47 +2029,62 @@ string YulUtilFunctions::copyValueArrayStorageToStorageFunction(ArrayType const&
 					let spill := sub(length, mul(fullSlots, <itemsPerSlot>))
 					if gt(spill, 0) {
 						let dstSlotValue := 0
-						<?sameType>
+						<?sameTypeFromStorage>
 							dstSlotValue := <maskBytes>(srcSlotValue, mul(spill, <srcStride>))
-							<updateSrcSlotValue>
-						<!sameType>
+							<updateSrcPtr>
+						<!sameTypeFromStorage>
 							for { let j := 0 } lt(j, spill) { j := add(j, 1) } {
-								let itemValue := <convert>(
+								<?isFromStorage>
+								let <stackItems> := <convert>(
 									<extractFromSlot>(srcSlotValue, mul(<srcStride>, srcItemIndexInSlot))
 								)
-								itemValue := <prepareStore>(itemValue)
+								<!isFromStorage>
+								let <stackItems> := <readFromMemoryOrCalldata>(srcPtr)
+								</isFromStorage>
+								let itemValue := <prepareStore>(<stackItems>)
 								dstSlotValue := <updateByteSlice>(dstSlotValue, mul(<dstStride>, j), itemValue)
 
-								<updateSrcSlotValue>
+								<updateSrcPtr>
 							}
-						</sameType>
+						</sameTypeFromStorage>
 						sstore(add(dstSlot, fullSlots), dstSlotValue)
 					}
 				</multipleItemsPerSlotDst>
 			}
 		)");
 		if (_fromType.dataStoredIn(DataLocation::Storage))
 			solAssert(!_fromType.isValueType(), "");
+
+		bool fromCalldata = _fromType.dataStoredIn(DataLocation::CallData);
+		bool fromStorage = _fromType.dataStoredIn(DataLocation::Storage);
 		templ("functionName", functionName);
 		templ("resizeArray", resizeArrayFunction(_toType));
 		templ("arrayLength", arrayLengthFunction(_fromType));
 		templ("panic", panicFunction(PanicCode::ResourceError));
+		templ("isFromDynamicCalldata", _fromType.isDynamicallySized() && fromCalldata);
+		templ("isFromStorage", fromStorage);
+		templ("readFromMemoryOrCalldata", readFromMemoryOrCalldata(*_fromType.baseType(), fromCalldata));
 		templ("srcDataLocation", arrayDataAreaFunction(_fromType));
 		templ("dstDataLocation", arrayDataAreaFunction(_toType));
 		templ("srcStride", to_string(_fromType.storageStride()));
+		templ("stackItems", suffixedVariableNameList(
+			"stackItem_",
+			0,
+			_fromType.baseType()->stackItems().size()
+		));
 		unsigned itemsPerSlot = 32 / _toType.storageStride();
 		templ("itemsPerSlot", to_string(itemsPerSlot));
 		templ("multipleItemsPerSlotDst", itemsPerSlot > 1);
-		bool sameType = *_fromType.baseType() == *_toType.baseType();
+		bool sameTypeFromStorage = fromStorage && (*_fromType.baseType() == *_toType.baseType());
 		if (auto functionType = dynamic_cast<FunctionType const*>(_fromType.baseType()))
 		{
 			solAssert(functionType->equalExcludingStateMutability(
 				dynamic_cast<FunctionType const&>(*_toType.baseType())
 			));
-			sameType = true;
+			sameTypeFromStorage = fromStorage;
 		}
-		templ("sameType", sameType);
-		if (sameType)
+		templ("sameTypeFromStorage", sameTypeFromStorage);
+		if (sameTypeFromStorage)
 		{
 			templ("maskFull", maskLowerOrderBytesFunction(itemsPerSlot * _toType.storageStride()));
 			templ("maskBytes", maskLowerOrderBytesFunctionDynamic());
@@ -2088,24 +2097,32 @@ string YulUtilFunctions::copyValueArrayStorageToStorageFunction(ArrayType const&
 			templ("convert", conversionFunction(*_fromType.baseType(), *_toType.baseType()));
 			templ("prepareStore", prepareStoreFunction(*_toType.baseType()));
 		}
-		templ("updateSrcSlotValue", Whiskers(R"(
-			<?srcReadMultiPerSlot>
-				srcItemIndexInSlot := add(srcItemIndexInSlot, 1)
-				if eq(srcItemIndexInSlot, <srcItemsPerSlot>) {
-					// here we are done with this slot, we need to read next one
-					srcSlot := add(srcSlot, 1)
-					srcSlotValue := sload(srcSlot)
-					srcItemIndexInSlot := 0
-				}
-			<!srcReadMultiPerSlot>
-				srcSlot := add(srcSlot, 1)
-				srcSlotValue := sload(srcSlot)
-			</srcReadMultiPerSlot>
-			)")
-			("srcReadMultiPerSlot", !sameType && _fromType.storageStride() <= 16)
-			("srcItemsPerSlot", to_string(32 / _fromType.storageStride()))
-			.render()
-		);
+		if (fromStorage)
+			templ("updateSrcPtr", Whiskers(R"(
+				<?srcReadMultiPerSlot>
+					srcItemIndexInSlot := add(srcItemIndexInSlot, 1)
+					if eq(srcItemIndexInSlot, <srcItemsPerSlot>) {
+						// here we are done with this slot, we need to read next one
+						srcPtr := add(srcPtr, 1)
+						srcSlotValue := sload(srcPtr)
+						srcItemIndexInSlot := 0
+					}
+				<!srcReadMultiPerSlot>
+					srcPtr := add(srcPtr, 1)
+					srcSlotValue := sload(srcPtr)
+				</srcReadMultiPerSlot>
+				)")
+				("srcReadMultiPerSlot", !sameTypeFromStorage && _fromType.storageStride() <= 16)
+				("srcItemsPerSlot", to_string(32 / _fromType.storageStride()))
+				.render()
+			);
+		else
+			templ("updateSrcPtr", Whiskers(R"(
+					srcPtr := add(srcPtr, <srcStride>)
+				)")
+				("srcStride", fromCalldata ? to_string(_fromType.calldataStride()) : to_string(_fromType.memoryStride()))
+				.render()
+			);
 
 		return templ.render();
 	});

libsolidity/codegen/YulUtilFunctions.h

@@ -258,9 +258,9 @@ class YulUtilFunctions
 	/// signature (to_slot, from_ptr) ->
 	std::string copyByteArrayToStorageFunction(ArrayType const& _fromType, ArrayType const& _toType);
 
-	/// @returns the name of a function that will copy an array of value types from storage to storage.
-	/// signature (to_slot, from_slot) ->
-	std::string copyValueArrayStorageToStorageFunction(ArrayType const& _fromType, ArrayType const& _toType);
+	/// @returns the name of a function that will copy an array of value types to storage.
+	/// signature (to_slot, from_ptr[, from_length]) ->
+	std::string copyValueArrayToStorageFunction(ArrayType const& _fromType, ArrayType const& _toType);
 
 	/// Returns the name of a function that will convert a given length to the
 	/// size in memory (number of storage slots or calldata/memory bytes) it

test/libsolidity/semanticTests/abiEncoderV1/struct/struct_storage_ptr.sol

@@ -24,6 +24,6 @@ contract C {
 // ----
 // library: L
 // f() -> 8, 7, 1, 2, 7, 12
-// gas irOptimized: 166606
+// gas irOptimized: 166525
 // gas legacy: 169347
 // gas legacyOptimized: 167269

test/libsolidity/semanticTests/abiEncoderV2/storage_array_encoding.sol

@@ -18,10 +18,10 @@ contract C {
 // EVMVersion: >homestead
 // ----
 // h(uint256[2][]): 0x20, 3, 123, 124, 223, 224, 323, 324 -> 32, 256, 0x20, 3, 123, 124, 223, 224, 323, 324
-// gas irOptimized: 180882
+// gas irOptimized: 180768
 // gas legacy: 184929
 // gas legacyOptimized: 181504
 // i(uint256[2][2]): 123, 124, 223, 224 -> 32, 128, 123, 124, 223, 224
-// gas irOptimized: 112533
+// gas irOptimized: 112471
 // gas legacy: 115468
 // gas legacyOptimized: 112988

test/libsolidity/semanticTests/array/arrays_complex_from_and_to_storage.sol

@@ -12,7 +12,7 @@ contract Test {
 }
 // ----
 // set(uint24[3][]): 0x20, 0x06, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12 -> 0x06
-// gas irOptimized: 189640
+// gas irOptimized: 186766
 // gas legacy: 211149
 // gas legacyOptimized: 206054
 // data(uint256,uint256): 0x02, 0x02 -> 0x09

test/libsolidity/semanticTests/array/constant_var_as_array_length.sol

@@ -9,7 +9,7 @@ contract C {
 
 // ----
 // constructor(): 1, 2, 3 ->
-// gas irOptimized: 141700
+// gas irOptimized: 141581
 // gas legacy: 183490
 // gas legacyOptimized: 151938
 // a(uint256): 0 -> 1

test/libsolidity/semanticTests/array/copying/array_copy_calldata_storage.sol

@@ -20,7 +20,7 @@ contract c {
 }
 // ----
 // store(uint256[9],uint8[3][]): 21, 22, 23, 24, 25, 26, 27, 28, 29, 0x140, 4, 1, 2, 3, 11, 12, 13, 21, 22, 23, 31, 32, 33 -> 32
-// gas irOptimized: 650748
+// gas irOptimized: 648324
 // gas legacy: 694515
 // gas legacyOptimized: 694013
 // retrieve() -> 9, 28, 9, 28, 4, 3, 32

test/libsolidity/semanticTests/array/copying/array_copy_cleanup_uint40.sol

@@ -46,6 +46,6 @@ contract C {
 }
 // ----
 // f() -> true
-// gas irOptimized: 146913
+// gas irOptimized: 146936
 // gas legacy: 155961
 // gas legacyOptimized: 153588

test/libsolidity/semanticTests/array/copying/array_copy_clear_storage.sol

@@ -13,6 +13,6 @@ contract C {
 }
 // ----
 // f() -> 0
-// gas irOptimized: 134352
+// gas irOptimized: 134365
 // gas legacy: 135313
 // gas legacyOptimized: 134548