Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

solidity: Remove the partial constant propagation #197

Open
xermicus opened this issue Feb 6, 2025 · 1 comment
Open

solidity: Remove the partial constant propagation #197

xermicus opened this issue Feb 6, 2025 · 1 comment

Comments

@xermicus
Copy link
Member

xermicus commented Feb 6, 2025

Reading through the YUL parser partial constant propagation implementation we can observe that constants are collected upon declarations. This seems incorrect or of little use since the declared identifier can later be assigned a different value (declaration does not imply a single static assignment).

With some debugging we can observe that:

This Solidity
  contract B {
      constructor() payable {
          payable(address(this)).transfer(msg.value);
      }
  }
compiles to tihs YUL
/// @use-src 0:"B.sol"
object "B_17" {
    code {
        {
            /// @src 0:0:100  "contract B {..."
            mstore(64, memoryguard(0x80))
            /// @src 0:49:91  "payable(address(this)).transfer(msg.value)"
            let _1 := 0
            if iszero(/** @src 0:81:90  "msg.value" */ callvalue())
            /// @src 0:49:91  "payable(address(this)).transfer(msg.value)"
            { _1 := 2300 }
            if iszero(call(_1, /** @src 0:65:69  "this" */ address(), /** @src 0:81:90  "msg.value" */ callvalue(), /** @src 0:49:91  "payable(address(this)).transfer(msg.value)" */ 0, 0, 0, 0))
            {
                /// @src 0:0:100  "contract B {..."
                let pos := mload(64)
                returndatacopy(pos, /** @src 0:49:91  "payable(address(this)).transfer(msg.value)" */ 0, /** @src 0:0:100  "contract B {..." */ returndatasize())
                revert(pos, returndatasize())
            }
            let _2 := mload(64)
            let _3 := datasize("B_17_deployed")
            codecopy(_2, dataoffset("B_17_deployed"), _3)
            return(_2, _3)
        }
    }
    /// @use-src 0:"B.sol"
    object "B_17_deployed" {
        code {
            {
                /// @src 0:0:100  "contract B {..."
                revert(0, 0)
            }
        }
        data ".metadata" hex"a264697066735822122020c842ef31aaa70d5e5cddae55e2f79e62f46f8887d02d13b74a8edfb3cf4ac464736f6c634300081c0033"
    }
}

which indeed leads to a constant being produced for identifier _1. Which is not correct as _1 changes it's value dynamically.

I vote for removing the related code because:

  • The feature appears to be buggy. The right place to implemented it is in another, transformative IR instead.
  • The feature appears to be unnecessary for most cases, as we expect LLVM to do constant propagation.
  • The feature is not required in the MVP.
@athei
Copy link
Member

athei commented Feb 6, 2025

Agreed. Let's get rid of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants