Unexpected SLOADs during storing tightly packed struct

[k06a]

Description

Unexpected SLOADs during storing tightly packed struct. The following code should produce 0 SLOADs and 2 SSTOREs from my point of view. But somehow it executes 1 SLOAD before SSTOREs.

Environment

• Compiler version: 0.8.11
• Target EVM version (as per compiler settings): default (runs=1000000)
• Framework/IDE (e.g. Truffle or Remix): Remix, Hardhat

Steps to Reproduce

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.11;

contract BitPacking {
    struct Data {
        uint48 a;
        uint40 b;
        uint40 c;
        uint128 d;
        uint128 e;
        uint128 f;
    }

    Data public data;

    function func(uint48 _1, uint40 _2, uint40 _3, uint128 _4) external {
        data = Data(_1, _2, _3, _4, _4, _4);
    }
}

[chriseth]

Solidity is still not very good at optimizing across memory stores (Data(...) creates the struct in memory). For now, the more efficient (and maybe also safer) way to do this is to use individual assignments - data.a = _1; data.b = _2; ... - since this does not create any copies in memory.

It is weird, though, that slot 0 is even read when doing individual assignments.

[k06a]

@chriseth I tried this code too, even before using structs. I have the same SLOADs and SSTOREs counts but no MLOADs and MSTOREs :)

[hrkrshnn]

Looked at the IR. The issue is that the function calls for writing to storage, corresponding to d, e and f are not getting inlined. I'd assume the same issue for the legacy codegen. I think improving the inlining heuristics should fix this.

[k06a]

@hrkrshnn what do you think about adding tests for such cases?

[chriseth]

I don't think we have an issue with inlining in the old codegen, because most routines are always inlined at code gen phase already. I would still like the sload of zero to be investigated.

[chriseth]

@k06a we already have tests for that and we know that the gas consumption is far from ideal.

I just re-checked the version where we store in struct members directly, i.e.

contract BitPacking {
    struct Data {
        uint48 a;
        uint80 b;
        uint128 c;
    }

    Data data;

    function func(uint48 _1, uint80 _2, uint128 _3) external {
        data.a = _1;
        data.b = _2;
        data.c = _3;
    }
}

And the code generated via ir does perform multiple stores, but it does not read from the storage slot for the final store (the multiple stores will be fixed by #11352 ).
The code via the old codegen does read from slot 0, but not if the first two members are combined. So I think this might just be a limitation of the old optimizer regarding code complexity.

[chriseth]

For reference, IR code:

            function fun_func(var, var_1, var_)
            {
                let _1 := and(var, 0xffffffffffff)
                let _2 := sload(0x00)
                sstore(0x00, or(and(_2, not(0xffffffffffff)), _1))
                let toInsert := and(shl(48, var_1), 0xffffffffffffffffffff000000000000)
                let _3 := not(0xffffffffffffffffffffffffffffffff)
                sstore(0x00, or(or(and(_2, _3), _1), toInsert))
                sstore(0x00, or(or(toInsert, _1), and(shl(128, var_), _3)))
            }

[k06a]

@chriseth what do you think about fixing and adding tests for such cases to avoid future regressions?

[ekpyron]

@chriseth what do you think about fixing and adding tests for such cases to avoid future regressions?

That's what we're doing :-). #11352 will fix the issue and will have appropriate tests, there are just some minor issues yet to be worked out to assure that it's always correct.

[k06a]

@ekpyron thanks, amazing!

[github-actions]

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

[github-actions]

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.