[mlir][llvmir] Translate function result attributes to LLVM IR.

[vzakhari]

Summary

Translate align, noalias, noundef, signext and zeroext result
attributes from llvm.func to LLVM IR.

This is needed for https://github.com/llvm/llvm-project/issues/58579

Depends on D137048

[ftynse]

LGTM with comments addressed.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

894–924

Could we move verification to LLVMDialect::verifyRegionResultAttribute?

mlir/test/Target/LLVMIR/llvmir-invalid.mlir

108 ↗

(On Diff #471871)

Nit: prefer @below to @+1.

[vzakhari]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

894–924

Yes, we should move it there. After looking through the verification code I think code here and in D137050 is not quite correct. These attributes should be return attributes, not the operation attributes. I will have to redo this and I will move verification along with this rewrite.

[vzakhari]

Hi @ftynse, there is some code duplication between verifyRegionArgAttribute and verifyRegionResultAttribute right now. I am going to get rid of it when addressing https://reviews.llvm.org/D137048#inline-1324059 in a separate commit.

[ftynse]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

894–924

Ultimately, argument/result attributes are operation attributes with a special name and pretty printing...

[vzakhari]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

894–924

I agree, in general, but at least the setters/getters are different for argument/result/operation attributes :)
Can you please take another look and see if everything looks okay to you?

[vzakhari]

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

2795

It looks like I cannot verify the argument/result type in the verification hooks. For example, there are cases where llvm.align is attached to memref<...> arguments, so it is not clear to me how to verify the value type here without converting the value type to LLVM dialect type. I suggest keeping the attribute's type (UnitAttr, IntegerAttr, etc.) verification here, but keep the value type verification inside LLVM dialect to LLVM IR translator (i.e. ModuleTranslation.cpp). @ftynse, does it sound appropriate to you?

[vzakhari]

Added LLVM::isCompatibleType() check to narrow the set of types that we can reliably diagnose in the verification code.

[vzakhari]

Hi @ftynse, does it look okay to you? Thank you!

[ftynse]

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

2795

This may be slightly better than before, so let's go with it, though the "verification" is now split between two places. I think attaching llvm.align to memref is plain wrong because the default convention converts memref to a struct with two pointers that have different alignment and _both_ will have the attribute in the LLVM representation. This is partly my fault for originally introducing llvm.noalias and not cleaning it up when the two-pointer approach was added (so now llvm.noalias on a memref is a blatant lie), but we should really model these things properly at higher levels or have interfaces that ensure they are interpreted correctly at lowering time.

[ftynse]

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

2795

Let me also tag @ezhulenev to whom I made a similar request in another patch.

[vzakhari]

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

2795

Thank you, @ftynse! Yes, I believe I saw that the integer members of the converted memref were also getting llvm.align, which was completely wrong.

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

 // - external functions have 'external' or 'extern_weak' linkage;
 // - vararg is (currently) only supported for external functions;
 LogicalResult LLVMFuncOp::verify() {
   if (getLinkage() == LLVM::Linkage::Common)
     return emitOpError() << "functions cannot have '"
                          << stringifyLinkage(LLVM::Linkage::Common)
                          << "' linkage";
 
-  // Check to see if this function has a void return with a result attribute to
-  // it. It isn't clear what semantics we would assign to that.
-  if (getFunctionType().getReturnType().isa<LLVMVoidType>() &&
-      !getResultAttrs(0).empty()) {
-    return emitOpError()
-           << "cannot attach result attributes to functions with a void return";
-  }
-
   if (isExternal()) {
     if (getLinkage() != LLVM::Linkage::External &&
         getLinkage() != LLVM::Linkage::ExternWeak)
       return emitOpError() << "external functions must have '"
                            << stringifyLinkage(LLVM::Linkage::External)
                            << "' or '"
                            << stringifyLinkage(LLVM::Linkage::ExternWeak)
                            << "' linkage";
LogicalResult LLVMDialect::verifyRegionArgAttribute(Operation *op,
   }
   return success();
 }
 
 LogicalResult LLVMDialect::verifyRegionResultAttribute(Operation *op,
                                                        unsigned regionIdx,
                                                        unsigned resIdx,
                                                        NamedAttribute resAttr) {
-  if (resAttr.getName() == LLVMDialect::getStructAttrsAttrName()) {
+  StringAttr name = resAttr.getName();
+  if (name == LLVMDialect::getStructAttrsAttrName()) {
     return verifyFuncOpInterfaceStructAttr(
         op, resAttr.getValue(), [resIdx](FunctionOpInterface funcOp) {
           return funcOp.getResultTypes()[resIdx];
         });
   }
+  if (auto funcOp = dyn_cast<FunctionOpInterface>(op)) {
+    mlir::Type resTy = funcOp.getResultTypes()[resIdx];
+
+    // Check to see if this function has a void return with a result attribute
+    // to it. It isn't clear what semantics we would assign to that.
+    if (resTy.isa<LLVMVoidType>())
+      return op->emitError() << "cannot attach result attributes to functions "
+                                "with a void return";
+
+    // LLVM attribute may be attached to a result of operation
+    // that has not been converted to LLVM dialect yet, so the result
+    // may have a type with unknown representation in LLVM dialect type
+    // space. In this case we cannot verify whether the attribute may be
+    // attached to a result of such type.
+    bool verifyValueType = isCompatibleType(resTy);
+    Attribute attrValue = resAttr.getValue();
+
+    // TODO: get rid of code duplication here and in verifyRegionArgAttribute().
+    if (name == LLVMDialect::getAlignAttrName()) {
+      if (!attrValue.isa<IntegerAttr>())
+        return op->emitError() << "expected llvm.align result attribute to be "
+                                  "an integer attribute";
+      if (verifyValueType && !resTy.isa<LLVMPointerType>())
+        return op->emitError()
+               << "llvm.align attribute attached to non-pointer result";
+      return success();
+    }
+    if (name == LLVMDialect::getNoAliasAttrName()) {
+      if (!attrValue.isa<UnitAttr>())
+        return op->emitError() << "expected llvm.noalias result attribute to "
+                                  "be a unit attribute";
+      if (verifyValueType && !resTy.isa<LLVMPointerType>())
+        return op->emitError()
+               << "llvm.noalias attribute attached to non-pointer result";
+      return success();
+    }
+    if (name == LLVMDialect::getNoUndefAttrName()) {
+      if (!attrValue.isa<UnitAttr>())
+        return op->emitError() << "expected llvm.noundef result attribute to "
+                                  "be a unit attribute";
+      return success();
+    }
+    if (name == LLVMDialect::getSExtAttrName()) {
+      if (!attrValue.isa<UnitAttr>())
+        return op->emitError() << "expected llvm.signext result attribute to "
+                                  "be a unit attribute";
+      if (verifyValueType && !resTy.isa<mlir::IntegerType>())
+        return op->emitError()
+               << "llvm.signext attribute attached to non-integer result";
+      return success();
+    }
+    if (name == LLVMDialect::getZExtAttrName()) {
+      if (!attrValue.isa<UnitAttr>())
+        return op->emitError() << "expected llvm.zeroext result attribute to "
+                                  "be a unit attribute";
+      if (verifyValueType && !resTy.isa<mlir::IntegerType>())
+        return op->emitError()
+               << "llvm.zeroext attribute attached to non-integer result";
+      return success();
+    }
+  }
+
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Utility functions.
 //===----------------------------------------------------------------------===//
 
 Value mlir::LLVM::createGlobalString(Location loc, OpBuilder &builder,

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

for (auto function : getModuleBody(mlirModule).getOps<LLVMFuncOp>()) {
     llvm::FunctionCallee llvmFuncCst = llvmModule->getOrInsertFunction(
         function.getName(),
         cast<llvm::FunctionType>(convertType(function.getFunctionType())));
     llvm::Function *llvmFunc = cast<llvm::Function>(llvmFuncCst.getCallee());
     llvmFunc->setLinkage(convertLinkageToLLVM(function.getLinkage()));
     mapFunction(function.getName(), llvmFunc);
     addRuntimePreemptionSpecifier(function.getDsoLocal(), llvmFunc);
 
+    // Convert function attributes.
     if (function->getAttrOfType<UnitAttr>(LLVMDialect::getReadnoneAttrName()))
       llvmFunc->setDoesNotAccessMemory();
 
+    // Convert result attributes.
+    if (ArrayAttr allResultAttrs = function.getAllResultAttrs()) {
+      llvm::AttrBuilder retAttrs(llvmFunc->getContext());
+      DictionaryAttr resultAttrs = allResultAttrs[0].cast<DictionaryAttr>();
+      for (const NamedAttribute &attr : resultAttrs) {
+        StringAttr name = attr.getName();
+        if (name == LLVMDialect::getAlignAttrName()) {
+          auto alignAmount = attr.getValue().cast<IntegerAttr>();
+          retAttrs.addAlignmentAttr(llvm::Align(alignAmount.getInt()));
+        } else if (name == LLVMDialect::getNoAliasAttrName()) {
+          retAttrs.addAttribute(llvm::Attribute::NoAlias);
+        } else if (name == LLVMDialect::getNoUndefAttrName()) {
+          retAttrs.addAttribute(llvm::Attribute::NoUndef);
+        } else if (name == LLVMDialect::getSExtAttrName()) {
+          retAttrs.addAttribute(llvm::Attribute::SExt);
+        } else if (name == LLVMDialect::getZExtAttrName()) {
+          retAttrs.addAttribute(llvm::Attribute::ZExt);
+        }
+      }
+      llvmFunc->addRetAttrs(retAttrs);
+    }
+
     // Convert argument attributes.
     unsigned int argIdx = 0;
     for (auto [mlirArgTy, llvmArg] :
          llvm::zip(function.getArgumentTypes(), llvmFunc->args())) {
       if (auto attr = function.getArgAttrOfType<UnitAttr>(
               argIdx, LLVMDialect::getNoAliasAttrName())) {
         // NB: Attribute already verified to be boolean, so check if we can
         // indeed attach the attribute to this argument, based on its type.
         if (!mlirArgTy.isa<LLVM::LLVMPointerType>())
           return function.emitError(
               "llvm.noalias attribute attached to LLVM non-pointer argument");
         llvmArg.addAttr(llvm::Attribute::AttrKind::NoAlias);
       }
 
       if (auto attr = function.getArgAttrOfType<IntegerAttr>(
               argIdx, LLVMDialect::getAlignAttrName())) {
         // NB: Attribute already verified to be int, so check if we can indeed
         // attach the attribute to this argument, based on its type.
         if (!mlirArgTy.isa<LLVM::LLVMPointerType>())

mlir/test/Dialect/LLVMIR/func.mlir

module {
   // expected-error@+1 {{failed to construct function type: expected zero or one function result}}
   llvm.func @foo() -> (i64, i64)
 }
 
 // -----
 
 module {
   // expected-error@+1 {{cannot attach result attributes to functions with a void return}}
-  llvm.func @variadic_def() -> (!llvm.void {llvm.noalias})
+  llvm.func @variadic_def() -> (!llvm.void {llvm.noundef})
 }
 
 // -----
 
+// expected-error @below{{expected llvm.align result attribute to be an integer attribute}}
+llvm.func @alignattr_ret() -> (!llvm.ptr {llvm.align = 1.0 : f32})
+
+// -----
+
+// expected-error @below{{llvm.align attribute attached to non-pointer result}}
+llvm.func @alignattr_ret() -> (i32 {llvm.align = 4})
+
+// -----
+
+// expected-error @below{{expected llvm.noalias result attribute to be a unit attribute}}
+llvm.func @noaliasattr_ret() -> (!llvm.ptr {llvm.noalias = 1})
+
+// -----
+
+// expected-error @below{{llvm.noalias attribute attached to non-pointer result}}
+llvm.func @noaliasattr_ret() -> (i32 {llvm.noalias})
+
+// -----
+
+// expected-error @below{{expected llvm.noundef result attribute to be a unit attribute}}
+llvm.func @noundefattr_ret() -> (!llvm.ptr {llvm.noundef = 1})
+
+// -----
+
+// expected-error @below{{expected llvm.signext result attribute to be a unit attribute}}
+llvm.func @signextattr_ret() -> (i32 {llvm.signext = 1})
+
+// -----
+
+// expected-error @below{{llvm.signext attribute attached to non-integer result}}
+llvm.func @signextattr_ret() -> (f32 {llvm.signext})
+
+// -----
+
+// expected-error @below{{expected llvm.zeroext result attribute to be a unit attribute}}
+llvm.func @zeroextattr_ret() -> (i32 {llvm.zeroext = 1})
+
+// -----
+
+// expected-error @below{{llvm.zeroext attribute attached to non-integer result}}
+llvm.func @zeroextattr_ret() -> (f32 {llvm.zeroext})
+
+// -----
+
 module {
   // expected-error@+1 {{variadic arguments must be in the end of the argument list}}
   llvm.func @variadic_inside(%arg0: i32, ..., %arg1: i32)
 }
 
 // -----
 
 module {

mlir/test/Target/LLVMIR/llvmir.mlir

 // CHECK-LABEL: define void @zeroextattr(i1 zeroext %
 llvm.func @zeroextattr(%arg0: i1 {llvm.zeroext}) {
   llvm.return
 }
 
 // CHECK-LABEL: declare void @zeroextattr_decl(i1 zeroext)
 llvm.func @zeroextattr_decl(i1 {llvm.zeroext})
 
+// CHECK-LABEL: declare align 4 ptr @alignattr_ret_decl()
+llvm.func @alignattr_ret_decl() -> (!llvm.ptr<i32> {llvm.align = 4})
+// CHECK-LABEL: declare noalias ptr @noaliasattr_ret_decl()
+llvm.func @noaliasattr_ret_decl() -> (!llvm.ptr<i32> {llvm.noalias})
+// CHECK-LABEL: declare noundef ptr @noundefattr_ret_decl()
+llvm.func @noundefattr_ret_decl() -> (!llvm.ptr<i32> {llvm.noundef})
+// CHECK-LABEL: declare signext i1 @signextattr_ret_decl()
+llvm.func @signextattr_ret_decl() -> (i1 {llvm.signext})
+// CHECK-LABEL: declare zeroext i1 @zeroextattr_ret_decl()
+llvm.func @zeroextattr_ret_decl() -> (i1 {llvm.zeroext})
+
 // CHECK-LABEL: @llvm_varargs(...)
 llvm.func @llvm_varargs(...)
 
 llvm.func @intpointerconversion(%arg0 : i32) -> i32 {
 // CHECK:      %2 = inttoptr i32 %0 to ptr
 // CHECK-NEXT: %3 = ptrtoint ptr %2 to i32
   %1 = llvm.inttoptr %arg0 : i32 to !llvm.ptr<i32>
   %2 = llvm.ptrtoint %1 : !llvm.ptr<i32> to i32