[mlir][llvm] Fixes CallOp builder for the case of indirect call

[gitoleg]

The problem.

The CallOp of the LLVM dialect has the next behavior: once the callee attribute is set, the call is considered as a direct one. And vice versa, once callee is not set, the call is an indirect one. For the case of indirect call, the first operand to the CallOp must be a function pointer, and all the remaining are arguments of the function. One of the CallOp builders doesn't actually check the callee attribute and always generate function type from the all operands, which later caused a fail in transition from the dialect to LLVM IR due to different number of operands expected by function type here. This PR fix it.

@ivanradanov

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-llvm

Author: None (gitoleg)

Changes

The problem.

The CallOp of the LLVM dialect has the next behavior: once the callee attribute is set, the call is considered as a direct one. And vice versa, once callee is not set, the call is an indirect one. For the case of indirect call, the first operand to the CallOp must be a function pointer, and all the remaining are arguments of the function. One of the CallOp builders doesn't actually check the callee attribute and always generate function type from the all operands, which later caused a fail in transition from the dialect to LLVM IR due to different number of operands expected by function type here. This PR fix it.

---

Full diff: https://github.com/llvm/llvm-project/pull/76240.diff

1 Files Affected:

• (modified) mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp (+2-1)

diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index 458bf83eac17f8..2fb82aeaa2bda7 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -908,8 +908,9 @@ void CallOp::build(OpBuilder &builder, OperationState &state, TypeRange results,
 
 void CallOp::build(OpBuilder &builder, OperationState &state, TypeRange results,
                    FlatSymbolRefAttr callee, ValueRange args) {
+  auto fargs = callee ? args : args.drop_front();
   build(builder, state, results,
-        TypeAttr::get(getLLVMFuncType(builder.getContext(), results, args)),
+        TypeAttr::get(getLLVMFuncType(builder.getContext(), results, fargs)),
         callee, args, /*fastmathFlags=*/nullptr, /*branch_weights=*/nullptr,
         /*CConv=*/nullptr,
         /*access_groups=*/nullptr, /*alias_scopes=*/nullptr,

[ivanradanov]

I wonder if it may be better to have an assert that the callee is present here, and instead provide a separate builder, which takes callee argument of type Value that constructs an indirect call.

This may be less confusing as the builders above that take a callee with string types always result in a direct call.

[ivanradanov]

But if there is a lot of code that depends on this behaviour then I guess this works as well.

[gysit]

I very much agree with Ivan that there should be an assert that checks that callee is non-null and possibly an alternative builder signature that does not take a callee. Alternatively, the function type could also be computed outside of the builder. However, I think it is fine to add a new builder signature that does this for us.

[gitoleg]

well, let's have one more round :) And I'll do the fix!

well, I have some doubts about the idea to pass a callee as a value ( or I just misunderstood something :) ) The reason is that everywhere the behavior is assumed as it stated in the docs:
with the first operand being a callee in the operand list in the case of indirect call. So passing a callee separately as a value may be another source of confusion - if the operands must contain the callee in this case or not. Let's just add a builder that doesn't have a callee as an argument - and it will be a true indirect call!

And one more thought about: the fact we don't want to check the attribute here means that it need to be checked in the user code - we just move the problem from this place to another. You can consider the ClangIR code that lowers CIR to MLIR. So choosing the proper builder would cause adding extra checks in there.

But of course it's up to you to decide. I would say that the crox of the problem is that we probably need two different operations for direct and indirect calls - in this case no confusion would happen.

So what's the plan?) What do I need to do?

@ivanradanov @gysit

[ivanradanov]

The docs refer to the operands of the MLIR op, and not the builder. The builders that take in a callee that specify a function using a symbol should result in a direct call IMO.

Let's just add a builder that doesn't have a callee as an argument - and it will be a true indirect call!

We already have builders do not take callee as an argument and produce an indirect call, but one needs to provide the callee type instead. This is because there is no way to infer whether the callee is variadic or not in the case of an indirect call.

In fact, I think that CIR code is wrong when the indirect callee is variadic (unless CIR has a different CallOp for variadic functions?)

If you want to retain the current functionality (which is wrong if you have indirect calls to variadic), I believe you can use the default builder (

llvm-project/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

Line 625 in d8ddcae

dag args = (ins OptionalAttr<TypeAttrOf<LLVM_FunctionType>>:$callee_type,

) and pass in nullptr as the callee_type. Which will make the lowering assume that the callee is non variadic.

[gitoleg]

well, the next problem will arise here: once the callee_type is not set, the function type will be inferred from the operands, and again, with a pointer to the function itself at the first place.

[ivanradanov]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:980

Operation::operand_range CallOp::getArgOperands() {
  return getOperands().drop_front(getCallee().has_value() ? 0 : 1);
}

We drop the first operand when callee_type is not set so it should be inferred correctly.

[gysit]

Let's land this then? @gitoleg do you have commit rights? Otherwise, one of us will land.

[gitoleg]

@gysit
no rights :)

We drop the first operand when callee_type is not set so it should be inferred correctly.

@ivanradanov yes, you're right!

[gysit]

ok, I am traveling today. Will land once I am back (@ivanradanov feel free to go ahead if you are around). The title and description of the PR is somewhat outdated but I think I can adapt it when landing.

[gysit]

Thanks LGTM!

[gysit]

Suggested change

	assert(callee && "no callee in direct call builder");
	assert(callee && "expected non-null callee in direct call builder");

ultra nit:

[gitoleg]

@gysit done!