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

Return type of void functions breaks MLIR invariants #1127

Open
keryell opened this issue Nov 15, 2024 · 2 comments
Open

Return type of void functions breaks MLIR invariants #1127

keryell opened this issue Nov 15, 2024 · 2 comments

Comments

@keryell
Copy link
Contributor

keryell commented Nov 15, 2024

I work on a project involving some CIR micro-surgery and I was banging my head on having mlir::inlineCall() working and it actually was due to the precondition on https://github.com/llvm/llvm-project/blob/main/mlir/lib/Transforms/Utils/InliningUtils.cpp#L458 failing: a
The reason is that C++ void (...) functions say they return a !cir.void type in CIR but actually they return no value.
The ClangIR infrastructure hides this in various ways.
Some random sampling:

bool FuncType::isVoid() const { return mlir::isa<VoidType>(getReturnType()); }

In clang/lib/CIR/Dialect/IR/CIRDialect.cpp:

ParseResult cir::FuncOp::parse() {
...
  // Fetch return type or set it to void if empty/ommited.
  mlir::Type returnType =
      (resultTypes.empty() ? cir::VoidType::get(builder.getContext())
                           : resultTypes.front());
}
static LogicalResult
verifyCallCommInSymbolUses(...) {
...
  // Void function must not return any results.
  if (fnType.isVoid() && op->getNumResults() != 0)
    return op->emitOpError("callee returns void but call has results");

  // Non-void function calls must return exactly one result.
  if (!fnType.isVoid() && op->getNumResults() != 1)
    return op->emitOpError("incorrect number of results for callee");

  // Parent function and return value types must match.
  if (!fnType.isVoid() &&
      op->getResultTypes().front() != fnType.getReturnType()) {
    return op->emitOpError("result type mismatch: expected ")
           << fnType.getReturnType() << ", but provided "
           << op->getResult(0).getType();

I thought about different ways to fix this but at the end I think the most reasonable solution is not to go against MLIR. 😄
Just remove the return type of void functions and adapt the infrastructure accordingly.
I have seen some breakage when using --remove-dead-values and --symbol-dce which might be related.
I am curious about how we make the difference in between K&R C f() and C99 f(void) but this is an orthogonal story.

@bcardosolopes
Copy link
Member

I think the most reasonable solution is not to go against MLIR.

I agree!

Just remove the return type of void functions and adapt the infrastructure accordingly.

Did you have any problems with function pointers to void? Another one would be casts to void to implicit use a value, etc.
Good thing we are at least advanced enough in the project such that we already cover most uses of void we know, so if everything passes we're probably good!

I have seen some breakage when using --remove-dead-values and --symbol-dce which might be related.

If you could add tests for those as well, we probably good overall to make the transition.

@keryell
Copy link
Contributor Author

keryell commented Nov 26, 2024

I have a local WIP on this. Kind of hairy but at least it compiles. 😉

Total Discovered Tests: 482
  Passed           : 343 (71.16%)
  Expectedly Failed:   4 (0.83%)
  Failed           : 135 (28.01%)

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

No branches or pull requests

2 participants