[mlir][Sol] Build precise per-type dispatch tables for internal fnptr calls

[abinavpp]

NomicFoundation/solx-solidity#172

[Copilot]

Pull request overview

This PR improves lowering of internal function-pointer indirect calls (sol.icall) by precomputing a precise candidate set per function-pointer type and generating out-of-line Yul dispatch functions keyed by that candidate set (and runtime/creation phase).

Changes:

• Annotates each sol.icall with a candidates attribute containing the ids of functions whose address is taken for that callee type.
• Lowers sol.icall by calling a shared out-of-line __sol.internal_dispatch.* helper that switches over the annotated candidate ids and panics on invalid ids.
• Tightens Sol op definitions for function constants / indirect calls and adds helper-symbol support for internal dispatch function naming.

Reviewed changes

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

Show a summary per file

File	Description
mlir/lib/Conversion/SolToYul/SolToYulPass.cpp	Adds a pass-entry walk that computes and annotates sol.icall candidate-id sets.
mlir/lib/Conversion/SolToYul/SolToYul.cpp	Reworks sol.icall lowering to generate/reuse out-of-line per-candidate-set dispatch functions.
mlir/lib/Conversion/SolToYul/EVMUtil.cpp	Adds helper-symbol name generation for internal dispatch functions.
mlir/include/mlir/Dialect/Sol/SolOps.td	Refines Sol op types and adds candidates attribute to sol.icall.
mlir/include/mlir/Conversion/SolToYul/EVMUtil.h	Declares the new internal-dispatch helper-symbol generator.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[abinavpp]

The indirect-call filter matches via-ir now as we only reference address-taken internal functions, but also surpasses it because we do the match on the original type, not the legalized arity (which via-ir does). So in some cases, the dispatch table will be smaller than via-ir, while staying correct. The outlining avoids penalizing the user for doing too many indirect-calls, I think it's a good call. Of course, llvm can later inline it if the dispatch is trivially small.

[PavelKopyl]

I'd suggest to create a task with the following possible issues of the dispatch tables, as our final goal is to reduce dispatch table sizes

1. Flow-insensitive per call site. Every icall of a type gets the union of all takes of that type.

p1 = f; p1();   // table {f, g}; only f can flow here
p2 = g; p2();   // table {f, g}; only g can flow here

Per-site minimality needs points-to analysis.
2. Phase-blind takes. Buckets don't record which phase took the value. Tables are filtered only by where the target exists. A function taken only at runtime but also directly called from the ctor gets a spurious creation-table entry:

constructor() { realRt(); p(); }     // cr table lists realRt
function take() public { p = realRt; }  // ...but the only take is runtime

3. No reachability pruning. Tables anchor dead code. A func_constant in a never-called function still contributes its id, and the table's symbol reference then keeps that dead function alive through DCE:
   function unused() internal { p = f; } // unreachable, yet f enters every table
4. Empty sets still get an outlined table, It's better to inline it.

[PavelKopyl]

The assert and the consequent if statement condition contradict to each other. I think we could use an llvm_unreachable here.

[PavelKopyl]

I suggest to move sorting from this loop to do sort only once, finalize sorted attribute creation and then use them to annotate icalls. We could do something like:

  DenseMap<std::pair<Operation *, sol::FuncRefType>, DenseI64ArrayAttr>
      bucketAttrs;
  for (auto &[key, idSet] : buckets) {
    SmallVector<int64_t> ids = idSet.takeVector();
    llvm::sort(ids);
    bucketAttrs[key] = b.getDenseI64ArrayAttr(ids);
  }

  DenseI64ArrayAttr emptyAttr = b.getDenseI64ArrayAttr({});
  for (sol::ICallOp icall : icalls) {
    auto it =
        bucketAttrs.find({icall->getParentOfType<ModuleOp>().getOperation(),
                          cast<sol::FuncRefType>(icall.getCallee().getType())});
    icall.setCandidatesAttr(it != bucketAttrs.end() ? it->second : emptyAttr);
  }

[PavelKopyl]

It looks like we may get a symbol collision for empty candidateIds. For example:

Contract Test {
function (uint256) internal returns (uint256) f1;
function () internal f2;
function a(uint256 x) public returns (uint256) { return f1(x); }
function b() public { f2(); }
}

causes a crash, as it produces one func.func @__sol.internal_dispatch.rt : (i256, i256) -> i256 and two incompatible call sites. I'd suggest to inline an empty dispatch function (that contains only Panic(0x51) call).

[PavelKopyl]

It looks like we have here an API mismatch: in TableGen candidates are declared as optional, OptionalAttr<DenseI64ArrayAttr>:$candidates meaning a sol.icall without this attribute is a fully valid op, but at this stage (to be more specific, after annotateICallCandidates) we require it's presence. We could introduce another opcode, ResolvedICallOp where this attribute is not optional. annotateICallCandidates would convert CallOp to the new operation and declaring CallOp invalid.

[PavelKopyl]

I'd suggest to add here more checks that we do not cause symbol clashing with:

• non functions
• functions, but with different signatures

Operation *found = SymbolTable::lookupSymbolIn(symTab, dispatchFnName);
auto dispatchFn = dyn_cast_or_null<yul::FuncOp>(found);
assert((!found || dispatchFn) && "dispatch symbol clashes with non-function");
assert(!dispatchFn || dispatchFn.getFunctionType() == expectedDispatchTy);

[PavelKopyl]

I suggest to add an assert that the enclosing function's ancestor is already a yul.object.