[Sol->Yul] Optimizing delete struct.

[mijovic]

Depends on #9843

[ekpyron]

I actually think this is safe, too, and could be merged right after (or together with) #9843.

@chriseth I think you asked for this to be extracted: do you have any concern here?

The main question is the last slot, so if

struct S1 { uint8 x; }

is changed to

struct S2 { uint8 x; uint8 y; }

Then this code here would already clear y as well when deleting S1, whereas the conservative method will keep an sload and mask even with #6728 . So in that sense this is an actual change in behavior - but any state variable after the struct will live in the next slot anyways, won't it?

Also #6728 won't kick in completely e.g. for the first slot of struct S { uint120 x; uint120 y; uint256 z; }, since x and y don't occupy the entire slot, so the masks won't cancel the sload out and the sload will stay. So if that's changed to struct S { uint120 x; uint120 y; uint8 a; uint256 z; }, but still deleted from code using the old definition, a will not be cleared, right?

I'd even argue that clearing all slots entirely is the better behavior for deleting structs - otherwise we could even miss deleting some members in contract upgrade situations, couldn't we? I'm not entirely sure though, it's a bit delicate... but only clearing members seems problematic to me as well in that sense.

What do you think?

[chriseth]

There should be an issue about this problem: Do we want to make use of the knowledge that in the current storage layout, some parts of a slot are completely unused?

One reason why we did not make use of that knowledge yet is because of upgrades: Someone might want to add or remove items at the end of a struct. If this is an issue here has to be discussed, but I would prefer not to just merge this without a decision on that subject.

I think we even discussed that in the past and concluded that the gas savings are not worth the trouble.

[ekpyron]

I'm aware of that discussion, but I don't think that's exactly the same - this here is really only about deleting the struct, so the situation is slightly different, isn't it? Here I'd argue we actually should delete the whole slot, particularly with an eye of upgrades...

[ekpyron]

I.e. if I extend struct S { uint8 a; } to struct S { uint8 a; uint8 b; } - is it then sane behavior that deleting the struct might ever not delete b?

[ekpyron]

You're referring to this: #7891, right? Where we decided against it in the end?

[chriseth]

Needs some copy editing.

[mijovic]

I changes this a bit, hope it looks better now

[chriseth]

Can you add some tests that mix short value types with dynamically-sized arrays, or also nested arrays, or do we already have such tests?

[mijovic]

@chriseth I added few tests that show where this is different to the old codegen. There are 3 different tests, and all of them fail with old codegen.

[ekpyron]

Not sure how much care we need to take for the ir-breaking-changes docs - we'll probably have to refine them again in the end either way and could think of them primarily as a reminder for now... so fine with merging it as it is, but I'll post some comments about the docs either way.

[ekpyron]

Maybe we should rather say "that old code generated for delete before adding that struct" or something like that, but I'd also say it's fine for now - we'll need to refine this in the end when we make it "public" anyways I expect.

[ekpyron]

Ah, you merged already anyways :-D.