Skip to content

[mlir][NVVM] Disallow results on kernel functions #96399

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

Merged
merged 1 commit into from
Jun 23, 2024

Conversation

matthias-springer
Copy link
Member

Functions that have the nvvm.kernel attribute should have 0 results.

Functions that have the `nvvm.kernel` attribute should have 0 results.
@llvmbot
Copy link
Member

llvmbot commented Jun 22, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-llvm

Author: Matthias Springer (matthias-springer)

Changes

Functions that have the nvvm.kernel attribute should have 0 results.


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

2 Files Affected:

  • (modified) mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp (+10-3)
  • (modified) mlir/test/Target/LLVMIR/nvvmir.mlir (+7)
diff --git a/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp
index 94197e473ce01..3d6a911f36541 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp
@@ -214,7 +214,8 @@ void MmaOp::print(OpAsmPrinter &p) {
   p.printOptionalAttrDict(this->getOperation()->getAttrs(), ignoreAttrNames);
 
   // Print the types of the operands and result.
-  p << " : " << "(";
+  p << " : "
+    << "(";
   llvm::interleaveComma(SmallVector<Type, 3>{frags[0].regs[0].getType(),
                                              frags[1].regs[0].getType(),
                                              frags[2].regs[0].getType()},
@@ -955,7 +956,9 @@ std::string NVVM::WgmmaMmaAsyncOp::getPtx() {
   ss << "},";
   // Need to map read/write registers correctly.
   regCnt = (regCnt * 2);
-  ss << " $" << (regCnt) << "," << " $" << (regCnt + 1) << "," << " p";
+  ss << " $" << (regCnt) << ","
+     << " $" << (regCnt + 1) << ","
+     << " p";
   if (getTypeD() != WGMMATypes::s32) {
     ss << ", $" << (regCnt + 3) << ",  $" << (regCnt + 4);
   }
@@ -1053,10 +1056,14 @@ LogicalResult NVVMDialect::verifyOperationAttribute(Operation *op,
   StringAttr attrName = attr.getName();
   // Kernel function attribute should be attached to functions.
   if (attrName == NVVMDialect::getKernelFuncAttrName()) {
-    if (!isa<LLVM::LLVMFuncOp>(op)) {
+    auto funcOp = dyn_cast<LLVM::LLVMFuncOp>(op);
+    if (!funcOp) {
       return op->emitError() << "'" << NVVMDialect::getKernelFuncAttrName()
                              << "' attribute attached to unexpected op";
     }
+    if (!funcOp.getResultTypes().empty()) {
+      return op->emitError() << "kernel function cannot have results";
+    }
   }
   // If maxntid and reqntid exist, it must be an array with max 3 dim
   if (attrName == NVVMDialect::getMaxntidAttrName() ||
diff --git a/mlir/test/Target/LLVMIR/nvvmir.mlir b/mlir/test/Target/LLVMIR/nvvmir.mlir
index a8ae4d97888c9..26ba80cba6ed5 100644
--- a/mlir/test/Target/LLVMIR/nvvmir.mlir
+++ b/mlir/test/Target/LLVMIR/nvvmir.mlir
@@ -574,3 +574,10 @@ llvm.func @kernel_func(%arg0: !llvm.ptr {llvm.byval = i32, nvvm.grid_constant})
 llvm.func @kernel_func(%arg0: !llvm.ptr {llvm.byval = i32, nvvm.grid_constant}, %arg1: f32, %arg2: !llvm.ptr {llvm.byval = f32, nvvm.grid_constant}) attributes {nvvm.kernel} {
   llvm.return
 }
+
+// -----
+
+// expected-error @below{{kernel function cannot have results}}
+llvm.func @kernel_with_result(%i: i32) -> i32 attributes {nvvm.kernel}  {
+  llvm.return %i : i32
+}

Copy link
Member

@grypp grypp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing that

@matthias-springer matthias-springer merged commit 346c4a8 into main Jun 23, 2024
10 checks passed
@matthias-springer matthias-springer deleted the users/matthias-springer/nvvm_verifier branch June 23, 2024 07:50
Mogball pushed a commit that referenced this pull request Jun 28, 2024
Mogball pushed a commit that referenced this pull request Jun 28, 2024
…97074)

NVVM IR itself doesn't place any restriction that a function annotated
as `nvvm.kernel` actually has no results, so this is a mismatch at the
NVVMDialect level and NVVMIR. The GPU dialect might enforce that kernel
functions have no results, but it doesn't make sense to propagate this
constraint downstream.

Reverts #96399
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
…" (llvm#97074)

NVVM IR itself doesn't place any restriction that a function annotated
as `nvvm.kernel` actually has no results, so this is a mismatch at the
NVVMDialect level and NVVMIR. The GPU dialect might enforce that kernel
functions have no results, but it doesn't make sense to propagate this
constraint downstream.

Reverts llvm#96399
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
Functions that have the `nvvm.kernel` attribute should have 0 results.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants