[mlir][Sol] Unify ABI tuple and packed encoders

[abinavpp]

NomicFoundation/solx-solidity#126

[Copilot]

Pull request overview

Adds sol::StructType handling to evm::Builder::genABIPackedEncoding, enabling packed ABI encoding of structs by iterating over members and reading them from calldata/memory/storage via StructEncodeMemberReader.

Changes:

• Implement struct-type branch in genABIPackedEncoding, selecting a member reader based on DataLocation.
• Encode each struct member into memory and advance the destination pointer accordingly (including recursive encoding for non-scalar members).

---

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

[vladimirradosavljevic]

Even though this PR adds struct handling in packed encoding to support events, I think we should add full support here, including array support.
Old codegen calls tupleEncoderPacked for indexed events. That sets options.dynamicInplace = true, which is not fully supported in the MLIR encoder yet. For example, array cases such as indexed arrays with reference elements still need more work.
It seems like now is a good time to address this properly. Maybe the best solution is to merge/generalize genABIPackedEncoding and genABITupleEncoding into one implementation with encoding options, e.g. padded and dynamicInplace, similar to how solc does it.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 46ececa84573af6e0f4d4fe0210e4fea8c58f28e d60cb9570cecb538b211a890cf1fad908d93c44e -- mlir/include/mlir/Conversion/SolToYul/EVMUtil.h mlir/include/mlir/Dialect/Sol/Sol.h mlir/lib/Conversion/SolToYul/EVMUtil.cpp mlir/lib/Conversion/SolToYul/SolToYul.cpp mlir/lib/Dialect/Sol/SolBase.cpp mlir/lib/Dialect/Sol/SolOps.cpp

View the diff from clang-format here.

diff --git a/mlir/lib/Conversion/SolToYul/EVMUtil.cpp b/mlir/lib/Conversion/SolToYul/EVMUtil.cpp
index bede8a1b6a4b..40473460a854 100644
--- a/mlir/lib/Conversion/SolToYul/EVMUtil.cpp
+++ b/mlir/lib/Conversion/SolToYul/EVMUtil.cpp
@@ -1852,8 +1852,7 @@ void evm::Builder::genClearStorageArrayTail(Value arraySlot,
 
   Type eltTy = arrTy.getEltType();
   // Multiple elements share one slot when byteSize <= 16 (elemsPerSlot >= 2).
-  bool trulyPacked =
-      sol::canBePacked(eltTy) && sol::getNumBytes(eltTy) <= 16;
+  bool trulyPacked = sol::canBePacked(eltTy) && sol::getNumBytes(eltTy) <= 16;
 
   yul::IfOp ifClear = nullptr;
   if (!isDecrement) {

[abinavpp]

Even though this PR adds struct handling in packed encoding to support events, I think we should add full support here, including array support.
Old codegen calls tupleEncoderPacked for indexed events. That sets options.dynamicInplace = true, which is not fully supported in the MLIR encoder yet. For example, array cases such as indexed arrays with reference elements still need more work.
It seems like now is a good time to address this properly. Maybe the best solution is to merge/generalize genABIPackedEncoding and genABITupleEncoding into one implementation with encoding options, e.g. padded and dynamicInplace, similar to how solc does it.

Thanks @vladimirradosavljevic, this makes sense. I've added an ABIEncodingOptions struct (padded, dynamicInplace) and unified everything behind a single genABIEncoding(...) API. wdyt?

[vladimirradosavljevic]

One comment before I do full review. Thanks for doing this!

[vladimirradosavljevic]

I think it would be beneficial to make this code more maintainable and readable. Could you please check whether we can achieve that?

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

[abinavpp]

I think it would be beneficial to make this code more maintainable and readable. Could you please check whether we can achieve that?

It's a bit more structured now. I still feel we can do a better job, especially with the array encoding. But I think that can wait.

[vladimirradosavljevic]

LGTM after two comments are addressed.
Thank you and awesome work!