Redundant store eliminator limited

[chriseth]

No description provided.

[k06a]

Hope this new PR will also help with this issue: #12460 (comment)

[chriseth]

Hope this new PR will also help with this issue: #12460 (comment)

In combination with #12206 it does address the issue, but it might also need some more careful tuning of the inlining parameters to make this always work.

[ekpyron]

Suggested change

	if (!funCall)
	return;
	yulAssert(funCall);

?

[ekpyron]

This reverted again now - but well, it also doesn't really matter too much.

[ekpyron]

Since we did set the lengths to constant 1 for these, maybe we should assert that they're there and that they are one here.

[chriseth]

Rebased.

[chriseth]

Squashed.

[ekpyron]

I wonder whether we should document this somewhere more prominently...
Also: did this have a significant impact beyond the literal-constant-only version? If not, I tend to play it safe and revert to that one... but if it has some effect, it's probably fine to go for it...

[chriseth]

It was able to optimize this out: https://github.com/ethereum/solidity/pull/12672/files#diff-454ca1e7741b290c37ead677fb52b645bd9c18325aa1aa5d023df0b926b2e1b7R16

I'm not sure what the impact in practice was. Of course, "known unrelated" is always difficult because we need the code to also terminate in the same function in order to say something about memory.

Still, if we want to improve this with the linear solver, we need exactly the same reasoning here.

[chriseth]

I just checked, that is the only change for isoltest.

[ekpyron]

I would have hoped that the linear solver could actually tell us stuff like if _op1.start <= _op2.start and then the problem just vanishes...

[ekpyron]

But it might not be able to do that accurately either if either of them could ever have overflowed...

[ekpyron]

It still seems a bit weird to me to have the optimizer operate under conflicting assumptions - we say both "large memory reads don't return values", but we also allow them to be load-resolved, so we also say "we can assume any memory read to return a value"...
I don't see any issue due to this in practice (yet?), but it will never be formally correct like this at least :-)...

[ekpyron]

And I actually wouldn't be surprised there was a case, in which this actually happens:

• a store to high memory location A
• a store to high memory location B
• a read from high memory location A

in which B is spuriously unrelated to A... then the write to B would be removed and we may resolve the read from A, after which we can potentially further remove the write to A...
Even if that doesn't happen the way things are set up right now for any concrete values of A and B, we'd have the constant danger of this happening, once we extend the optimizer anywhere...

[chriseth]

Ok, the interaction with the load resolver is actually a good point.

[ekpyron]

To do this stuff properly, maybe there is no way around proving that the free memory pointer and arithmetic on it never overflows... but that's probably always going to be tricky (especially across functions calls and such)...

[ekpyron]

On the other hand: any code we generate in which the free memory pointer does overflow is ill-defined anyways (since we may then remove all kinds of memory writes and reads preventing actual out-of-gas, but have a write after the memory pointer overflow mess with the scratch space or the free memory pointer itself)...

[chriseth]

The termination conditions are still wrong...

[ekpyron]

LGTM apart from remaining minor coding style comments.

[ekpyron]

Hm... this could be
knownToBeDifferentByAtLeast(*_op.start, *_op2.start, max(*length1, *length2))
thinking of two mstore8s...

And there's nothing really that restricts this case to only <= 32, is there?

[ekpyron]

Probably <= 2^255 is the only boundary, if any?

[chriseth]

That is true, but I'm not sure it is worth it delaying the PR :)

[bshastry]

The fuzzer reported the following trace divergence (mstore optimized out and hence the keccak256 call returns a different value)

{
  mstore(add("z",iszero(0x9f)), 0xa0)
  sstore(0xa1, 0xff)
  sstore(0x100, 0x101)
  sstore(keccak256("z",0x10), 0x100)
}

[ekpyron]

@bshastry Actually, the keccak256 seems to be evaluated even before this PR... so this resolves to

{
            mstore("z", 0xa0)
            sstore(0xa1, 0xff)
            let _1 := 0x100
            sstore(_1, 0x101)
            sstore(110620294328144418057589324861608220015688365608948720310623173341503153578932, _1)
}

after which the mstore is really redundant and can be removed...

and it's actually correct - we know the value stored in memory at "z" is 0x00...00a0, so the first 0x10 bytes are all zero and the big constant should be the keccak256 hash of 16 zeroes...

So it may be a false positive, but I'm not sure where it's coming from... maybe the interpreter (that's what you use as reference, right?) calculates the keccak hash incorrectly?

[ekpyron]

Ah, the yul interpreter doesn't interpret reads and writes from high memory addresses, does it?
EVMInstructionInterpreter::accessMemory looks like it only allows access up to numeric_limits<size_t>::max() - 0xffff...

And because of that EVMInstructionInterpreter::eval returns the bogus return u256("0x1234cafe1234cafe1234cafe") + arg[0]; in L185 - and won't get the keccak value right... I think that's it...

[ekpyron]

Not sure if there's good reason to restrict the interpreter to only low memory accesses... maybe it should be a setting...