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

[ethdebug] Transport debug data through common subexpression eliminator. #15088

Draft
wants to merge 3 commits into
base: yul_transport_debugdata_attributes_to_assembly_items
Choose a base branch
from

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented May 9, 2024

Depends on #14969 and #15009.

(#15009 is just cherry-picked here and should be removed once merged)

See https://notes.ethereum.org/lznAP49lRj6zrLJdLpkqwg.

CommonSubExpressionEliminator

Effect

{
    let x := 42
    let y := add(sload(42), calldataload(0))
    ....
    let z := 42
    ....
    
    f(42, add(sload(42), calldataload(0)))
}

---->

{
    let x := 42
    let y := add(sload(42), calldataload(0))
    ....
    let z := x
    ...    
    f(x, y)
}

Arbitrarily complex expressions that we know to evaluate to a value currently in a yul variable can be replaced by a yul variable. This can happen through complex control flow (into branches, across loops, etc.) and across arbitrary distances.

Relevance

Very High. This is one of the main aspects of moving things around together with Rematerializing (see below). Especially when done in sequence, we loose a strict division between variable and value. For the optimizer the concepts fuse.
TODO: need to elaborate in more complex examples. There may appear to be simple solutions like only preserving the debugging info of the replaced expression, but in more complex examples it should become apparent that the distinction cannot be reasonably kept up across multiple optimizations.

Implementation

TBD.

@aarlt aarlt added the has dependencies The PR depends on other PRs that must be merged first label May 9, 2024
{
let a := /* @debug.set {"assignment":"a"} */ mul(1, codesize()) /* @debug.set {} */
let b := /* @debug.set {"assignment":"b"} */ mul(1, codesize()) /* @debug.set {} */
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekpyron What result would you expect for this example? I would basically just merge the debug attributes here, basically creating a list of possible items:

// ----
// step: commonSubexpressionEliminator
//
// {
//     let a := /* @debug.set {"assignment": ["a", "b"]} */ mul(1, codesize()) /* @debug.set {} */
//     let b := /* @debug.set {"assignment": ["a", "b"]} */ a /* @debug.set {} */
// }
//
// Assembly:
//     /* "":58:68   */
//   codesize  // @debug.set {"assignment": ["a", "b"]}
//     /* "":55:56   */
//   0x01  // @debug.set {"assignment": ["a", "b"]}
//     /* "":51:69   */
//   mul  // @debug.set {"assignment": ["a", "b"]}
//     /* "":139:157   */
//   dup1  // @debug.set {"assignment": ["a", "b"]}
//     /* "":0:179   */
//   pop
//   pop

I think a debugger could easily find and use relevant information.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I'm not very sure that this is correct in the general case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this would be more useful:

{
    let a := /* @debug.set {"assignment":"a", "data": 1} */ mul(1, codesize()) /* @debug.set {} */
    let b := /* @debug.set {"assignment":"b", "data": 2} */ mul(1, codesize()) /* @debug.set {} */
}
// ----
// step: commonSubexpressionEliminator
//
// {
//     let a := /* @debug.set [{"assignment":"a", "data": 1}, {"assignment":"b", "data": 2}] */ mul(1, codesize()) /* @debug.set {} */
//     let b := /* @debug.set [{"assignment":"a", "data": 1}, {"assignment":"b", "data": 2}] */ a /* @debug.set {} */
// }
//
// Assembly:
//     /* "":58:68   */
//   codesize  // @debug.set [{"assignment":"a", "data": 1}, {"assignment":"b", "data": 2}]
//     /* "":55:56   */
//   0x01  // @debug.set [{"assignment":"a", "data": 1}, {"assignment":"b", "data": 2}]
//     /* "":51:69   */
//   mul  // @debug.set [{"assignment":"a", "data": 1}, {"assignment":"b", "data": 2}]
//     /* "":139:157   */
//   dup1  // @debug.set [{"assignment":"a", "data": 1}, {"assignment":"b", "data": 2}]
//     /* "":0:179   */
//   pop
//   pop

}
// ----
// step: commonSubexpressionEliminator
//
// {
// let a := mul(1, codesize())
// let a := /** @debug.set {"assignment":"a"} */ mul(1, codesize())
// let b := a
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm.. something seem not to work.. would have expected

// let b := /** @debug.set [{"assignment":"a"}, {"assignment":"b"}] */ a

here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has dependencies The PR depends on other PRs that must be merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant