[MLIR][LLVM] Don't use void return type in `getCallableResults`.

[definelicht]

Summary

In the LLVM IR dialect, LLVMVoidType is used to model the return type
of LLVM IR functions with no return value. This is inconsistent with
MLIR APIs, which expect a function with no return value to have an
empty return type. Handle this special case in LLVMFuncOp to avoid
mismatches between the number of return values and return types between
caller and callee.

[ftynse]

I'm not convinced by this change. It creates unclear special casing, e.g., calling the getResults() on the LLVMFunctionOp will return different results than calling getFunctionType().getResults(). The parser is not expected to see !llvm.void, but the type builder is. There is no mechanism left to check the presence of the void result type textually.

Have you considered (a) removing the void type entirely, (b) relaxing whatever caller/callee constraints to accommodate this?

[definelicht]

In D141676#4051432, @ftynse wrote:

I'm not convinced by this change. It creates unclear special casing, e.g., calling the getResults() on the LLVMFunctionOp will return different results than calling getFunctionType().getResults(). The parser is not expected to see !llvm.void, but the type builder is. There is no mechanism left to check the presence of the void result type textually.

Have you considered (a) removing the void type entirely, (b) relaxing whatever caller/callee constraints to accommodate this?

For context, this came up while extending the LLVMInlinerInterface (see stacked commit). Functions with no return type failed inlining, which turned out to stem from callResults.size() != callableResultTypes.size() between the LLVM::CallOp and the LLVM::LLVMFuncOp in InliningUtils.cpp. Unfortunately I don't see an easy fix for this, since mlir::InlineCall does not (and should not) know about - and special case on - the LLVM dialect.

[ftynse]

I get the context. It doesn't justify the added complexity and special casing here, and there are multiple ways forward.

I do agree with you that inlineCall should not know about LLVM dialect. The usual way around this restriction in MLIR is to use interfaces. We can extend one of the interfaces involved (Inliner, CallOp, CallableOp) to provide the mapping between the result types of the callable object and the results of the call operation, when this mapping is not one-to-one (one-to-one shouldn't necessarily be a requirement, like it isn't for branching terminators). This mapping can then be used instead of the more strict check on the equality of the number of results.

Alternatively, we can limit the change to getCallableResults to return the list of types that is different from getResultTypes. There doesn't seem to be a requirement for those to match, but I didn't check.

Alternatively, we can eliminate LLVMVoidType completely from the type system and use the empty list of results in the function type instead.

[definelicht]

In D141676#4056136, @ftynse wrote:

Alternatively, we can eliminate LLVMVoidType completely from the type system and use the empty list of results in the function type instead.

I'm giving this a try. We still need to somehow deal with the fact that all LLVM dialect APIs expect the return type to be a singular Type. I see a few approaches:

• Burninate LLVMVoidType. Use the empty type (impl == nullptr, which can be checked with !myType) as a sentinel value indicating a void return type. Don't use this to populate the return type.
• Burninate LLVMVoidType. Update all APIs to be Optional<Type> to represent the sentinel value. Don't use this to populate the return type.
• Keep LLVMVoidType around as the sentinel value, but don't use it to populate the result type.

Do you have any insights/preferences?

[definelicht]

I made an attempt at burninating LLVMVoidType and using an unconstructed/null Type, but this quickly seemed like a bad idea: It's still relying on a sentinel type, but a much less explicit one, and it breaks the assumption that we can access APIs on Types all over the code. This seems like an overall reduction in code readability.

Our intuition is now that the only way to truly get rid of LLVMVoidType (without just implicitly renaming it to something else) would be to fully adopt the MLIR convention of always having an array of return types, but doing our best to make sure that this is never set to >1 types. This is a trade-off that is not immediately convincing: we'd now instead need to validate that the size of this array is 0 or 1 at every entry and exit point of the LLVM dialect infrastructure, and would be constructing/passing around a lot of arrays that we know will only ever be size 0 or 1.

For now I will restrict this change to only update getCallableResults to at least adhere to the CallableOp API.

[definelicht]

Restrict change to getCallableResults. Don't touch parsing.

[ftynse]

Thanks for investigating!

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

let extraClassDeclaration = [{
 
     /// Returns the result types of this function.
     ArrayRef<Type> getResultTypes() { return getFunctionType().getReturnTypes(); }
 
     /// Returns the callable region, which is the function body. If the function
     /// is external, returns null.
     Region *getCallableRegion();
 
-    /// Returns the callable result type, which is the function return type.
-    ArrayRef<Type> getCallableResults() { return getFunctionType().getReturnTypes(); }
+    /// Returns the callable result type, which is the single function return
+    /// type if it is not void, or an empty array if the function's return type
+    /// is void, as void is not assignable to a value.
+    ArrayRef<Type> getCallableResults() {
+      if (getFunctionType().getReturnType().isa<LLVM::LLVMVoidType>())
+        return {};
+      return getFunctionType().getReturnTypes();
+    }
 
   }];
 
   let hasCustomAssemblyFormat = 1;
   let hasVerifier = 1;
   let hasRegionVerifier = 1;
 }