Yul builtin for MCOPY

[cameel]

First part of #14741.
Depends on #14790.

Still to do:

• Fix RedundantAssignEliminator wrongly removing MCOPY in some cases.
• Account for gas due to memory expansion of both read and write.
• More semantic tests, especially for the optimizer.
  • RedundantAssignEliminator
  • EqualStoreEliminator
  • yulInterpreterTest for MCOPY memory expansion.
  • Full suite
  • Memory guard

[cameel]

EqualStoreEliminator on the other hand seems to ignore mcopy.

[ekpyron]

Just FYI: I also just looked through potential effects on the LoadResolver and such - ultimately memory is tracked by the DataFlowAnalyzer there, though, which on each ExpressionStatement (that's not a simple mstore or sstore) that has sideEffects.invalidatesMemory() (which should include mcopy), properly clear knowledge about memory, so that part looks fine!

[ekpyron]

I also just looked through the code, to see if an mcopy will prevent an unmarked assembly block from being implicitly considered "memory-safe", resulting in a memoryguard being emitted - but that relies on _inlineAssembly.annotation().hasMemoryEffects, which should be correctly set based on the defined memory side-effects. It wouldn't hurt to add a memoryGuardTest, though (just with a simple assembly { mcopy(0, 1, 2) } or whatever making sure that won't result in a memoryguard being emitted)

[cameel]

Good point about memory guard tests. I haven't actually considered that.

For now I added extensive tests for mcopy in unused store eliminator and equal store eliminator in isolation, but I guess I should also add at least some full suite tests to catch anything that might be done by other steps I did not consider.

[cameel]

The annoying thing here is that I had to manually create variables for constants here despite the fact that we run ExpressionSplitter and SSATransform in the test framework just before testing the step. Without this, the first sstore was not being removed and it makes me worried that it could happen UnusedStoreEliminator tests I added (i.e. that stores would remain untouched not because behavior is correct but because the extra transformations prevent it).

The way it is now, the code gets transformed into this just before we run UnusedStoreEliminator:

    {
        let _1 := 0x40
        let _2 := 0
        let _3 := 0
        calldatacopy(_3, _2, _1)
        let _0 := 0
        let _32 := 32
        let _42 := 42
        let _123 := 123
        sstore(_0, _42)
        mcopy(_32, _0, _32)
        sstore(_0, _123)
        let _4 := 0x40
        let _5 := 0
        return(_5, _4)
    }

and the first sstore gets removed by the step.

but when the input looked like this:

    ...
    sstore(0, 42)
    mcopy(32, 0, 32)
    sstore(0, 123)
    ...

it would instead end up like this before UnusedStoreEliminator:

    {
        let _1 := 0x40
        let _2 := 0
        let _3 := 0
        calldatacopy(_3, _2, _1)
        let _4 := 42
        let _5 := 0
        sstore(_5, _4)
        let _6 := 32
        let _7 := 0
        let _8 := 32
        mcopy(_8, _7, _6)
        let _9 := 123
        let _10 := 0
        sstore(_10, _9)
        let _11 := 0x40
        let _12 := 0
        return(_12, _11)
    }

and both sstores would remain.

Unfortunately this means that these tests ended up being more verbose than they could be otherwise.

[nikola-matic]

Running CommonSubexpressionEliminator before UnusedStoredEliminator will get rid of the redundant sstore by the way. Obviously, injecting this into the test suite will likely yield changes in a lot of tests, which is not ideal, but we do indeed run CSE multiple times before the UnusedStoreEliminator . I'll try to explain, although take everything I say with a grain of salt until Daniel (or maybe even Chris) chime in. The following is the above Yul code transformed after ExpressionSplitter and SSATransform are run:

{
    let _1 := 0x40
    let _2 := 0
    let _3 := 0
    calldatacopy(_3, _2, _1)
    let _4 := 42
    let _5 := 0
    sstore(_5, _4)
    let _6 := 32
    let _7 := 0
    let _8 := 32
    mcopy(_8, _7, _6)
    let _9 := 123
    let _10 := 0
    sstore(_10, _9)
}

Only pay attention to _2 and the destination for sstore writes, in the above case, _5 for the first one, and _10 for the second. After CommonSubexpressionEliminator is run, we get the following:

{
    let _1 := 0x40
    let _2 := 0
    let _3 := _2
    calldatacopy(_2, _2, _1)
    let _4 := 42
    let _5 := _2
    sstore(_2, _4)
    let _6 := 32
    let _7 := _2
    let _8 := _6
    mcopy(_6, _2, _6)
    let _9 := 123
    let _10 := _2
    sstore(_2, _9)
}

Notice that destinations for both sstores have changed from _5 and _10 to _2. UnusedStoreEliminator checks whether all code paths revert in order to remove an sstore (not relevant in this case), and whether all code paths lead to the first sstore being overwritten by a subsequent one (definitely relevant in this case). As you can see (after running the CSE), the second sstore will clearly overwrite the first sstore (_2), which is why it will then remove the first one as it's marked as redundant. This is not possible (in this case) without running the CSE, since without it, the UnusedStoreEliminator has no way to know whether _5 and _10 are the same destination (technically it does, but that's CSE's job).

Having written this up, I'm now fairly surprised we're not running the CommonSubexpressionEliminator for UnusedEliminatorTests (nor are we suggesting it in the docs).

This is what the code looks like afterwards by the way:

{
    let _1 := 0x40
    let _2 := 0
    let _3 := _2
    calldatacopy(_2, _2, _1)
    let _4 := 42
    let _5 := _2
    let _6 := 32
    let _7 := _2
    let _8 := _6
    mcopy(_6, _2, _6)
    let _9 := 123
    let _10 := _2
    sstore(_2, _9)
}

[nikola-matic]

Now that I think about it, the above could use at least the UnusedPruner to clean it up a bit, so I guess the assumption for UnusedStoreEliminator tests is that the provided code is already in a 'post CSE` form (which is a terrible assumption to be honest, since you have to do so manually as you've done here). All in all, it's gonna make review this incredibly annoying :)

[cameel]

(which is a terrible assumption to be honest, since you have to do so manually as you've done here)

On the other hand these test suites are meant to test a single step. So cramming too many steps into it, especially the more complex ones, might not be a good idea. You're always risking that it will hide a bug. Not sure about CSE specifically - may or may not be acceptable. We already run quite a few others before and after it.

In any case, it sounds like CSE should indeed be listed in the docs as a soft dependency. We should confirm with Daniel it makes sense and add it.

All in all, it's gonna make review this incredibly annoying :)

I mean, it's mostly annoying due to the extra verbosity. The test themselves are still pretty simple and you can pretty much ignore the declarations when reading since the names reflect the values.

[nikola-matic]

On the other hand these test suites are meant to test a single step. So cramming too many steps into it, especially the more complex ones, might not be a good idea. You're always risking that it will hide a bug. Not sure about CSE specifically - may or may not be acceptable. We already run quite a few others before and after it.

Yup, that's why left the second comment after giving it some thought.

In any case, it sounds like CSE should indeed be listed in the docs as a soft dependency. We should confirm with Daniel it makes sense and add it.

Yeah, or maybe we can get some input from @chriseth if he's not off as well?

[ekpyron]

I should also add at least some full suite tests to catch anything that might be done by other steps I did not consider.

We'll probably end up having more behaviour implicitly covered by tests than we had testing these components in total before ;-). Meaning: more tests won't hurt, but we also need to be mindful that a lot of this cannot be exhaustively tested, so we have to additionally rely on fuzzing and code path review as well.

[ekpyron]

As for code path review: I haven't found anything in the Yul optimizer yet.
We shouldn't forget the libevmasm optimizer, though - I'll have a quick look there as well. EDIT: I think libevmasm/KnownState.cpp:L182 catching MCOPY as memory writing operation should be enough there.

[cameel]

Meaning: more tests won't hurt, but we also need to be mindful that a lot of this cannot be exhaustively tested,

These tests are still not even close to being exhaustive. I did try to err on the side of including too many tests rather than missing an important one, but I still ended up dropping like 2/3 of those I originally wanted to add. For example I dropped most of the tests checking the removal of MCOPY because in the end that should not happen.

It's hard to get a good balance though. I may have gone a bit overboard, but I really didn't want to risk adding too few and causing another important bug in unused store remover. The fact that I originally broke it in the PR made me really wary of this. Also I missed that overlap problem in the interpreter - in that case I assumed that a single test would be enough and I was wrong :)

[cameel]

Not sure what to do about this test. It won't pass soltest_all because you cannot use msize() with optimizer enabled. I could easily add an option to skip a semantic test when optimizer is enabled, but the fact that we don't have such an obvious option already makes me think that there could be some reason behind it.

On the other hand, this is really testing evmone's behavior rather than anything substantial in our implementation so maybe we're fine without it. I have gas tests, which check this indirectly, by comparing the cost of memory expansion so it is covered in that way already. I did like this one better though because it actually shows in which cases expansion happens.

[cameel]

For now I added a fixup that removes the test.

[cameel]

I think we should have this kind of test for all the Cancun opcodes - BLOBBASEFEE, BLOBHASH. TSTORE, TLOAD. The GasMeter changes in other PRs are simple, so should be ok, but still, we don't have any coverage for them otherwise.

[cameel]

All the changes are done now. This can be now reviewed. Not marking as reviewable yet only because it depends on #14790. Also, I need to deal with the failing test (#14779 (comment)), but that will not really affect the PR in a significant way.

[nikola-matic]

I've gone over everything, and would feel comfortable approving, but it would be nice for someone else to double check the gas and semantic tests (especially the ones with a lot of copying and memory dumps involved). The optimizer tests I've gone over in detail and they look good (or at least as good as they can in the current state, i.e. without MCOPY not being a removal candidate).

[nikola-matic]

If it affects MSIZE, is this then not a candidate for removal?

[cameel]

Honestly, I haven't verified this so I could be wrong, but I assume it still is as long as your code does not use msize (and using that basically disables the optimizer). If you're not using msize then changing how much the memory is expanded only really changes gas usage (and potentially whether you contract will revert with out of gas or not). These are fair game for optimization.

Still, in this example the mcopy instruction does not really affect msize since calldatacopy already expands memory. The comment was more to indicate that it is the only thing mcopy of this form could affect. I'll remove that bit to avoid confusion.