eof: Add support for SWAPN/DUPN #15845
Conversation
ekpyron
force-pushed
the
eofAddSwapnDupn
branch
from
32eb1c6 to
98b1df3
Compare
|
I only skimmed over it for now, but one thing that's probably missing is semantic info. Shouldn't the new opcodes at the very least be included in |
ekpyron
force-pushed
the
eofAddSwapnDupn
branch
from
98b1df3 to
311d421
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ekpyron
force-pushed
the
eofAddSwapnDupn
branch
from
311d421 to
e409ed8
Compare
ekpyron
force-pushed
the
eofAddSwapnDupn
branch
from
e409ed8 to
606ebe5
Compare
There was a problem hiding this comment.
One actual problem I found is that just adding the new items to isDupInstruction()/isSwapInstruction() does not seem to be enough for the optimizer to handle them properly. A few more changes are needed.
Other than that the usual stuff: tests, docstrings, asserts and some suggestions for consistency.
| virtual void appendSwapN(size_t _depth) = 0; | ||
| virtual void appendDupN(size_t _depth) = 0; |
There was a problem hiding this comment.
Need docstrings here.
One very important thing to to mention is the range of _depth.
Would probably be useful to have that included in the docstrings in AssemblyItemType as well since this is the other main place where we seem to be documenting these things.
There was a problem hiding this comment.
We should also assert against the range in the implementation. We do have asserts in assembleEOF() already but IMO that's a bit disconnected and we should be checking that as the precondition in these Assembly helpers as well.
There was a problem hiding this comment.
We should also assert against the range in the implementation. We do have asserts in
assembleEOF()already but IMO that's a bit disconnected and we should be checking that as the precondition in theseAssemblyhelpers as well.
Should we :-)? What if we get a new version of EOF that extends the range? I can add them, but I'm not entirely convinced :-).
There was a problem hiding this comment.
I added asserts a few levels deeper in AssemblyItem, that should do it.
There was a problem hiding this comment.
Well, at least the check for >=1 should be fairly universal.
For the upper bound, fair enough, I guess from the viewpoint of the abstract assembly the limitation to uint8 is still a bit arbitrary and you could argue that only assembleEOF() we really hit something that concretely depend on it (the ability to fit the number into a single byte).
In any case the asserts you added seem fine.
There was a problem hiding this comment.
I'd still mention the 1-based indexing in AssemblyItemType docstrings though.
ekpyron
force-pushed
the
eofAddSwapnDupn
branch
6 times, most recently
from
9c42ce1 to
c39219d
Compare
ekpyron
force-pushed
the
eofAddSwapnDupn
branch
from
c39219d to
95d30b4
Compare
|
Looks like it needs some squashing of review fixes before merge. |
|
There were quite a few optional side-quests we decided to do separately or leave for later. Here's a list so that they're easier to find:
|
Needs a second look - I just wrote exactly what I needed for #15844