Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EVM] Refactor StackSlot std::variant -> LLVM style RTTI #760

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

akiramenai
Copy link
Collaborator

No description provided.

@akiramenai akiramenai requested a review from PavelKopyl January 21, 2025 19:54
Copy link
Contributor

@vladimirradosavljevic vladimirradosavljevic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please run clang-format?

llvm/lib/Target/EVM/EVMStackSlotBuilder.h Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackSlotBuilder.h Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackShuffler.h Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackShuffler.h Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackModel.h Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackModel.h Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackDebug.h Outdated Show resolved Hide resolved
@akiramenai akiramenai force-pushed the dborisenkov-stackslot branch from 5ba5646 to 14349a2 Compare January 23, 2025 09:30
@akiramenai
Copy link
Collaborator Author

clang-format is done.

Copy link
Contributor

@vladimirradosavljevic vladimirradosavljevic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran tests, and I saw some issues:

  1. Use-after-free issue (explained in the comment).
  2. tests/solidity/simple/loop/complex/1.sol is failing with Variable not found on stack, where previously was failing with too deep in the stack.
  3. tests/solidity/simple/algorithm/floating_point_simulation/geometry/lines.sol is failing with too deep in the stack, where previously was passing.

@akiramenai akiramenai force-pushed the dborisenkov-stackslot branch from 9a9b417 to 10bf2ff Compare January 23, 2025 19:55
@PavelKopyl
Copy link
Contributor

As we have switched to pointer semantics, I think it's reasonable to add const qualifier to StackSlot pointer definitions where this semantically required and also removing auto. At least this will simplify the code reading.

@akiramenai
Copy link
Collaborator Author

The same tests are failing now in the base branch and the PR.
So the patch is ready for another review iteration.

Copy link
Contributor

@vladimirradosavljevic vladimirradosavljevic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Just a comment to reduce repeated hash lookups (we have up to 3 per call currently).

Comment on lines +265 to +267
if (LiteralStorage.count(V) == 0)
LiteralStorage[V] = std::make_unique<LiteralSlot>(V);
return LiteralStorage[V].get();
Copy link
Contributor

@vladimirradosavljevic vladimirradosavljevic Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (LiteralStorage.count(V) == 0)
LiteralStorage[V] = std::make_unique<LiteralSlot>(V);
return LiteralStorage[V].get();
auto [It, Inserted] = LiteralStorage.try_emplace(V, nullptr);
if (Inserted)
It->second = std::make_unique<Literal>(V);
return It->second.get();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also applies to methods below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep the original implementation even if optimizer doesn't manage to remove the second lookup. It's shorter and better readable.

Copy link
Contributor

@PavelKopyl PavelKopyl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@PavelKopyl
Copy link
Contributor

Please, rebase also both stackifcation branches into the main.

@akiramenai akiramenai force-pushed the dborisenkov-stackslot branch from 4c3d171 to 8e178da Compare January 27, 2025 20:54
@akiramenai akiramenai merged commit 2b78f15 into ef-stackification Jan 28, 2025
@akiramenai akiramenai deleted the dborisenkov-stackslot branch January 28, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants