Skip to content

[mlir][Sol] Support packed and unpacked string storage layouts#19

Open
PavelKopyl wants to merge 1 commit intomainfrom
kpv-support-string-storage
Open

[mlir][Sol] Support packed and unpacked string storage layouts#19
PavelKopyl wants to merge 1 commit intomainfrom
kpv-support-string-storage

Conversation

@PavelKopyl
Copy link
Contributor

@PavelKopyl PavelKopyl commented Feb 23, 2026

This also adds support for:

  • Short strings (length < 32 bytes, packed encoding)
  • Long strings (length >= 32 bytes, unpacked encoding)
  • String operations: push(), push(x), pop(), length, indexing
  • Conversions between memory and storage representations

@PavelKopyl PavelKopyl marked this pull request as draft February 23, 2026 01:06
@PavelKopyl PavelKopyl force-pushed the kpv-support-string-storage branch from 3d5f103 to a1963c3 Compare February 23, 2026 17:44
@github-actions
Copy link

github-actions bot commented Feb 23, 2026 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

@PavelKopyl PavelKopyl force-pushed the kpv-support-string-storage branch from a1963c3 to a026c6e Compare February 23, 2026 17:59
@PavelKopyl PavelKopyl requested review from abinavpp and vladimirradosavljevic and removed request for vladimirradosavljevic February 23, 2026 18:01
@PavelKopyl PavelKopyl marked this pull request as ready for review February 23, 2026 19:33
@PavelKopyl PavelKopyl requested a review from Copilot February 23, 2026 19:38
Copilot AI reviewed Feb 23, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the Sol dialect lowering to support Solidity bytes/string storage layouts (short in-slot encoding vs long out-of-place encoding) and introduces dedicated string push operations to model push, push(x), pop, length, indexing, and storage↔memory conversions in the Sol→Yul pipeline.

Changes:

  • Add sol.push_string and sol.push_string_void ops and lower them to new EVM/Yul builder helpers.
  • Implement storage-specific string helpers in evm::Builder (length decode, push/pop, indexing, copy to/from memory/storage).
  • Extend GEP handling to account for storage reference types (string/struct) and wire new ops into the conversion pass.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
mlir/lib/Dialect/Sol/SolOps.cpp Adds PushStringVoidOp builder; expands storage ref-type handling in GepOp::build.
mlir/lib/Conversion/SolToStandard/SolToYul.cpp Adds lowering patterns for the new string push ops; updates pop/GEP/copy paths for strings.
mlir/lib/Conversion/SolToStandard/SolToStandardPass.cpp Marks new string push ops illegal so they are lowered in stage1.
mlir/lib/Conversion/SolToStandard/EVMUtil.cpp Adds substantial string storage/memory logic (length decode, push/pop, indexing, copy).
mlir/include/mlir/Dialect/Sol/SolOps.td Declares sol.push_string / sol.push_string_void.
mlir/include/mlir/Conversion/SolToStandard/Util.h Adds BuilderExt::genCeilDivision helper.
mlir/include/mlir/Conversion/SolToStandard/EVMUtil.h Exposes new evm::Builder APIs for string operations and generic copy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@PavelKopyl PavelKopyl force-pushed the kpv-support-string-storage branch from a026c6e to fc415e9 Compare February 24, 2026 00:23
abinavpp
abinavpp reviewed Feb 24, 2026
View reviewed changes
Copy link
Collaborator

@abinavpp abinavpp left a comment

Choose a reason for hiding this comment

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

Thanks a lot! The impl looks nice! Wish we could figure out a way to remove the ad-hoc push ops somehow.

mlir/include/mlir/Conversion/SolToStandard/EVMUtil.h
std::optional<Location> locArg = std::nullopt);
/// Generates length of a string.
mlir::Value
genStringLength(mlir::Value lengthSlot, mlir::sol::DataLocation dataLoc,
Copy link
Collaborator

@abinavpp abinavpp Feb 24, 2026

Choose a reason for hiding this comment

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

Do we have to make this public when we have genDynSize? The existing public api's are more or less type agnostic and we should try following that pattern for consistency

Copy link
Contributor Author

@PavelKopyl PavelKopyl Feb 25, 2026

Choose a reason for hiding this comment

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

Actually I use this function in SolToYul.cpp to avoid double reading from storage. In many cases we need bot the full slot value that encodes length and the extracted length itself.

mlir/lib/Dialect/Sol/SolOps.cpp Outdated
getDataLocation(baseAddrTy));

// Don't generate pointers to reference types in storage.
// FIXME: have we listed all the types?
Copy link
Collaborator

@abinavpp abinavpp Feb 24, 2026

Choose a reason for hiding this comment

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

how about using sol::isNonPtrRefType instead?

Copy link
Contributor Author

@PavelKopyl PavelKopyl Feb 25, 2026

Choose a reason for hiding this comment

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

Done.

mlir/lib/Conversion/SolToStandard/EVMUtil.cpp
return b.create<LLVM::ExtractValueOp>(loc, i256Ty, addr,
b.getDenseI64ArrayAttr({1}));

Value sizeSlot = genLoad(addr, dataLoc, loc);
Copy link
Collaborator

@abinavpp abinavpp Feb 24, 2026

Choose a reason for hiding this comment

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

Can we move this under the if block below to avoid the potential double genLoad

Copy link
Contributor Author

@PavelKopyl PavelKopyl Feb 25, 2026

Choose a reason for hiding this comment

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

Done, thank you!

@PavelKopyl PavelKopyl force-pushed the kpv-support-string-storage branch from fc415e9 to 4771d13 Compare February 25, 2026 00:44
@PavelKopyl
Copy link
Contributor Author

PavelKopyl commented Feb 25, 2026

Thanks a lot! The impl looks nice! Wish we could figure out a way to remove the ad-hoc push ops somehow.

Thank you!. I've removed PushStringVoidOp and extended PushOp.

@PavelKopyl PavelKopyl force-pushed the kpv-support-string-storage branch 2 times, most recently from 60bc3d5 to 5996b3b Compare February 25, 2026 17:29
@PavelKopyl PavelKopyl requested a review from abinavpp February 25, 2026 17:29
@PavelKopyl PavelKopyl force-pushed the kpv-support-string-storage branch from 5996b3b to fb8689c Compare February 26, 2026 01:17
@PavelKopyl
[mlir][Sol] Support packed and unpacked string storage layouts
1514d34 
This also adds support for:
- Short strings (length < 32 bytes, packed encoding)
- Long strings (length >= 32 bytes, unpacked encoding)
- String operations: push(), push(x), pop(), length, indexing
- Conversions between memory and storage representations
@PavelKopyl PavelKopyl force-pushed the kpv-support-string-storage branch from fb8689c to 1514d34 Compare February 26, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@vladimirradosavljevic vladimirradosavljevic Awaiting requested review from vladimirradosavljevic

@abinavpp abinavpp Awaiting requested review from abinavpp

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PavelKopyl @abinavpp