-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[mlir][nvvm]Add support for grid_constant attribute on LLVM function arguments #78228
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
Changes from all commits
28cdab8
bccf82b
af6966e
132fa6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
#ifndef MLIR_TARGET_LLVMIR_LLVMTRANSLATIONINTERFACE_H | ||
#define MLIR_TARGET_LLVMIR_LLVMTRANSLATIONINTERFACE_H | ||
|
||
#include "mlir/Dialect/LLVMIR/LLVMDialect.h" | ||
#include "mlir/IR/BuiltinAttributes.h" | ||
#include "mlir/IR/DialectInterface.h" | ||
#include "mlir/Support/LogicalResult.h" | ||
|
@@ -25,6 +26,7 @@ class IRBuilderBase; | |
namespace mlir { | ||
namespace LLVM { | ||
class ModuleTranslation; | ||
class LLVMFuncOp; | ||
} // namespace LLVM | ||
|
||
/// Base class for dialect interfaces providing translation to LLVM IR. | ||
|
@@ -58,6 +60,16 @@ class LLVMTranslationDialectInterface | |
LLVM::ModuleTranslation &moduleTranslation) const { | ||
return success(); | ||
} | ||
|
||
/// Hook for derived dialect interface to translate or act on a derived | ||
/// dialect attribute that appears on a function parameter. This gets called | ||
/// after the function operation has been translated. | ||
virtual LogicalResult | ||
convertParameterAttr(LLVM::LLVMFuncOp function, int argIdx, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of LLVM uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are you referring to? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
also most of the former is in tests or tools. That being said, I'm not a proponent of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do you suggest to fix this: I see it as a bug and I am consistently using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to have some clarity on what's the convention... Has anything came out after https://discourse.llvm.org/t/rfc-coding-standards-prefer-int-for-regular-arithmetic-use-unsigned-only-for-bitmask-and-when-you-intend-to-rely-on-wrapping-behavior/52191 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not seems like we'll be able to make anything a LLVM-wide policy here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MLIR side we could. I'm rather pro not making it worse though (e.g., I'd consider consistency when it's towards direction we'd want to go rather than where we ended up in undesirable position and keeping going down that route) |
||
NamedAttribute attr, | ||
LLVM::ModuleTranslation &moduleTranslation) const { | ||
return success(); | ||
} | ||
}; | ||
|
||
/// Interface collection for translation to LLVM IR, dispatches to a concrete | ||
|
@@ -90,6 +102,22 @@ class LLVMTranslationInterface | |
} | ||
return success(); | ||
} | ||
|
||
/// Acts on the given function operation using the interface implemented by | ||
/// the dialect of one of the function parameter attributes. | ||
virtual LogicalResult | ||
convertParameterAttr(LLVM::LLVMFuncOp function, int argIdx, | ||
NamedAttribute attribute, | ||
LLVM::ModuleTranslation &moduleTranslation) const { | ||
if (const LLVMTranslationDialectInterface *iface = | ||
getInterfaceFor(attribute.getNameDialect())) { | ||
return iface->convertParameterAttr(function, argIdx, attribute, | ||
moduleTranslation); | ||
} | ||
function.emitWarning("Unhandled parameter attribute '" + | ||
attribute.getName().str() + "'"); | ||
return success(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure the default here should be success. This would be dropping any attribute from a dialect without interface on the floor without warning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will modify it to return failure. I followed what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning failure causes several test failures. There are dialect attributes like 'fir.bindc_name' which doesn't require any handling here. I updated it to emit a warning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (unresolving, @ftynse should take another look here and acknowledge the solution explicitly) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, the analogy is fair enough. |
||
} | ||
}; | ||
|
||
} // namespace mlir | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an attribute for a kernel parameter, while other attributes are for the kernel itself. We might need to split them later