[Sol->Yul] Implementing delete struct

[mijovic]

I am sneaking in support for recursive functions in MultiUseYulFunctionCollector as it is needed for recursive structs. If there is some better way to solve this, I am willing to change that :)

Depends on #9914

[mijovic]

Add test that deletes array of structs

[ekpyron]

Some minor comments and remarks, but in general this looks good now.
I think this mainly depends on #9914 now for the recursive struct test? I'll look at that next in any case.

[mijovic]

I think #9914 will help activating recursive struct case. So, once it is merged, will do another check for that case.

[mijovic]

After merging #9914 I needed to implement conversion from Struct storage to Struct storage pointer in order to get recursive_struct_2.sol passing.
It activated bunch of tests as well :)

[chriseth]

Can you extract this part into its own PR?

[mijovic]

You mean the whole function?

[chriseth]

The first optimizing case.

[mijovic]

I removed it from here. Will open another PR with optimization.

[mijovic]

#9985 is where I extracted optimizing case. Also it targets this branch as it depends on this one.

[ekpyron]

One tiny coding style comment, but otherwise I'd say this is good to go (and due to the tinyness of the comment I'm already approving).

[mijovic]

@ekpyron Can you re-approve :)

[ekpyron]

Reapproving.

[chriseth]

Is there no way to ensure that at compile time?

[mijovic]

Here it is not possible actually. I'll check all the places where this is called. If we can ensure there, maybe we can remove this check.

[chriseth]

Yeah, I see the problem. Maybe let's keep it then and try to tune the optimizer.

[mijovic]

Hm, all the places where this is called it is set either from C++ variable which can be checked for equal to zero in case of ref types, or is hardcoded to 0
Maybe it is the best to remove checks at all?

[ekpyron]

The proper way to avoid the check here would be to have another version of this that just doesn't take an offset as argument and call that whenever the offset is statically zero - but that'd be quite some refactoring and complication in several layers above this - so I'd just keep the check and hope for the optimizer for now.

[chriseth]

So maybe we could revamp the function parameter removal steps we already have.

[ekpyron]

all non-reverting code paths of a function set a return variable to a certain constant value -> remove the return variable from the function and set it at call sites (similar to return variable elimination)
all calls to a function set a parameter to the same constant value: remove the parameter and assign the

We could widen this from "constant values" to "movable value of low cost", if we're careful enough.

[ekpyron]

I mean ideally this would even generalize to splitting any

function f() -> a, b {
  {
    a := ...expensive calculation...
  }
  {
    b := ...expensive calculation...
  }
}

Into

function f() -> a, b { a := f1() b := f2() }
function f1() -> a { a := ... }
function f2() -> b { b := ... }

which in the constant case (e.g. a := 42) will have f1() and then f inlined and reduce to moving constants out that way...
I think that may be the most reasonable alternative to the "outlining" idea we once had...

[ekpyron]

But it'll be tricky to figure out when this is possible, since in general the calculations will be interleaved, etc. So probably best to find a reasonable subset of cases to start - but maybe more general than just constants :-).

[ekpyron]

If we follow that path, this would be headed towards the old optimizer reconstructing values based on data flow analysis :-). But that's not necessarily a bad thing IMO.

[ekpyron]

Just because we were messing with this simultanously, so maybe otherwise the comment will get lost ;-):
I think this rather belongs in L2184 right when the function signature is first created.

[ekpyron]

(Then based on _from.sizeOnStack() and _to.sizeOnStack() - but that's the place that will really be hit, if they're not both one)

[mijovic]

I added it. But does it really makes sense, as there is a call before this:

if (_from.sizeOnStack() != 1 || _to.sizeOnStack() != 1)
	return conversionFunctionSpecial(_from, _to);

maybe solAssert instead?

[ekpyron]

Ah, I didn't see that - that's a rather unfortunate distinction and scatters the array conversions all over the place... we should think about changing this in the future, but not in this PR :-).
But we can just drop the assertion in this case, it was overdoing it anyways.

[mijovic]

Ah, I didn't see that - that's a rather unfortunate distinction and scatters the array conversions all over the place... we should think about changing this in the future, but not in this PR :-).
But we can just drop the assertion in this case, it was overdoing it anyways.

Yes, it looks very unfortunate...
Once I finish copying of arrays to storage will maybe take a look at this and do the refactor.

[ekpyron]

I looked through this five times now, because I'm a bit suspicious because I expected this to enable more conversion-related-tests apart from the deletion tests, but it probably depends on some of the others and I think it's fine now, I hope I'm not missing anything :-).

[chriseth]

I think most of the tests start with copying to storage, and that's one thing we have not implemented yet.

[ekpyron]

I think most of the tests start with copying to storage, and that's one thing we have not implemented yet.

Yeah, I hope so, too.