[mlir] Add support for recursive elements in DICompositeTypeAttr. #74948
Conversation
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir Author: Justin Wilson (waj334) ChangesImplements mutable storage for DICompositeTypeAttr in order to allow for self-references in its elements array. When the "identifier" parameter set non-empty, only this string participates in the hash key, though the storage is implemented such that only the "elements" parameter is mutable. The module translator will now create the respective instance of llvm::DICompositeType without elements and then it will call "llvm::DICompositeType::replaceElements" to set the elements after each element is translated. The only required IR change was that elements are explicitly wrapped in square brackets for the sake of parsing. Patch is 28.44 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/74948.diff 6 Files Affected:
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
index 5a65293a113c7..0aed5b7840fbe 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
@@ -342,27 +342,6 @@ def LLVM_DICompileUnitAttr : LLVM_Attr<"DICompileUnit", "di_compile_unit",
let assemblyFormat = "`<` struct(params) `>`";
}
-//===----------------------------------------------------------------------===//
-// DICompositeTypeAttr
-//===----------------------------------------------------------------------===//
-
-def LLVM_DICompositeTypeAttr : LLVM_Attr<"DICompositeType", "di_composite_type",
- /*traits=*/[], "DITypeAttr"> {
- let parameters = (ins
- LLVM_DITagParameter:$tag,
- OptionalParameter<"StringAttr">:$name,
- OptionalParameter<"DIFileAttr">:$file,
- OptionalParameter<"uint32_t">:$line,
- OptionalParameter<"DIScopeAttr">:$scope,
- OptionalParameter<"DITypeAttr">:$baseType,
- OptionalParameter<"DIFlags", "DIFlags::Zero">:$flags,
- OptionalParameter<"uint64_t">:$sizeInBits,
- OptionalParameter<"uint64_t">:$alignInBits,
- OptionalArrayRefParameter<"DINodeAttr">:$elements
- );
- let assemblyFormat = "`<` struct(params) `>`";
-}
-
//===----------------------------------------------------------------------===//
// DIDerivedTypeAttr
//===----------------------------------------------------------------------===//
@@ -675,6 +654,61 @@ def LLVM_AliasScopeDomainAttr : LLVM_Attr<"AliasScopeDomain",
let assemblyFormat = "`<` struct(params) `>`";
}
+//===----------------------------------------------------------------------===//
+// DICompositeTypeAttr
+//===----------------------------------------------------------------------===//
+
+def LLVM_DICompositeTypeAttr : LLVM_Attr<"DICompositeType", "di_composite_type",
+ /*traits=*/[NativeTypeTrait<"IsMutable">], "DITypeAttr"> {
+ let parameters = (ins
+ OptionalParameter<"unsigned">:$tag,
+ OptionalParameter<"StringAttr">:$name,
+ OptionalParameter<"DIFileAttr">:$file,
+ OptionalParameter<"uint32_t">:$line,
+ OptionalParameter<"DIScopeAttr">:$scope,
+ OptionalParameter<"DITypeAttr">:$baseType,
+ OptionalParameter<"DIFlags", "DIFlags::Zero">:$flags,
+ OptionalParameter<"uint64_t">:$sizeInBits,
+ OptionalParameter<"uint64_t">:$alignInBits,
+ OptionalArrayRefParameter<"DINodeAttr">:$elements,
+ OptionalParameter<"StringRef">:$identifier
+ );
+ let hasCustomAssemblyFormat = 1;
+ let genStorageClass = 0;
+ let storageClass = "DICompositeTypeAttrStorage";
+ let builders = [
+ AttrBuilder<(ins
+ "unsigned":$tag,
+ "StringAttr":$name,
+ "DIFileAttr":$file,
+ "uint32_t":$line,
+ "DIScopeAttr":$scope,
+ "DITypeAttr":$baseType,
+ "DIFlags":$flags,
+ "uint64_t":$sizeInBits,
+ "uint64_t":$alignInBits,
+ "::llvm::ArrayRef<DINodeAttr>":$elements
+ )>,
+ AttrBuilder<(ins
+ "StringRef":$identifier,
+ "unsigned":$tag,
+ "StringAttr":$name,
+ "DIFileAttr":$file,
+ "uint32_t":$line,
+ "DIScopeAttr":$scope,
+ "DITypeAttr":$baseType,
+ "DIFlags":$flags,
+ "uint64_t":$sizeInBits,
+ "uint64_t":$alignInBits,
+ CArg<"::llvm::ArrayRef<DINodeAttr>", "{}">:$elements
+ )>
+ ];
+ let extraClassDeclaration = [{
+ static DICompositeTypeAttr getIdentified(MLIRContext *context, StringRef identifier);
+ void replaceElements(const ArrayRef<DINodeAttr>& elements);
+ }];
+}
+
//===----------------------------------------------------------------------===//
// AliasScopeAttr
//===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h
index c370bfa2b733d..c38bf1c66bba3 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h
@@ -23,6 +23,10 @@
namespace mlir {
namespace LLVM {
+namespace detail {
+ struct DICompositeTypeAttrStorage;
+} // namespace detail
+
/// This class represents the base attribute for all debug info attributes.
class DINodeAttr : public Attribute {
public:
diff --git a/mlir/lib/Dialect/LLVMIR/IR/AttrDetail.h b/mlir/lib/Dialect/LLVMIR/IR/AttrDetail.h
new file mode 100644
index 0000000000000..fd02af42a3fc3
--- /dev/null
+++ b/mlir/lib/Dialect/LLVMIR/IR/AttrDetail.h
@@ -0,0 +1,140 @@
+//===- AttrDetail.h - Details of MLIR LLVM dialect attributes --------*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains implementation details, such as storage structures, of
+// MLIR LLVM dialect attributes.
+//
+//===----------------------------------------------------------------------===//
+#ifndef DIALECT_LLVMIR_IR_ATTRDETAIL_H
+#define DIALECT_LLVMIR_IR_ATTRDETAIL_H
+
+#include "mlir/Dialect/LLVMIR/LLVMAttrs.h"
+#include "mlir/IR/Types.h"
+
+namespace mlir {
+namespace LLVM {
+namespace detail {
+
+//===----------------------------------------------------------------------===//
+// DICompositeTypeAttrStorage
+//===----------------------------------------------------------------------===//
+
+struct DICompositeTypeAttrStorage : public ::mlir::AttributeStorage {
+ using KeyTy = std::tuple<unsigned, StringAttr, DIFileAttr, uint32_t,
+ DIScopeAttr, DITypeAttr, DIFlags, uint64_t, uint64_t,
+ ArrayRef<DINodeAttr>, StringRef>;
+
+ DICompositeTypeAttrStorage(unsigned tag, StringAttr name, DIFileAttr file,
+ uint32_t line, DIScopeAttr scope,
+ DITypeAttr baseType, DIFlags flags,
+ uint64_t sizeInBits, uint64_t alignInBits,
+ ArrayRef<DINodeAttr> elements,
+ StringRef identifier = StringRef())
+ : tag(tag), name(name), file(file), line(line), scope(scope),
+ baseType(baseType), flags(flags), sizeInBits(sizeInBits),
+ alignInBits(alignInBits), elements(elements), identifier(identifier) {}
+
+ unsigned getTag() const { return tag; }
+ StringAttr getName() const { return name; }
+ DIFileAttr getFile() const { return file; }
+ uint32_t getLine() const { return line; }
+ DIScopeAttr getScope() const { return scope; }
+ DITypeAttr getBaseType() const { return baseType; }
+ DIFlags getFlags() const { return flags; }
+ uint64_t getSizeInBits() const { return sizeInBits; }
+ uint64_t getAlignInBits() const { return alignInBits; }
+ ArrayRef<DINodeAttr> getElements() const { return elements; }
+ StringRef getIdentifier() const { return identifier; }
+
+ /// Returns true if this attribute is identified.
+ bool isIdentified() const {
+ if (identifier.empty())
+ return false;
+
+ return !identifier.empty();
+ }
+
+ /// Returns the respective key for this attribute.
+ KeyTy getAsKey() const {
+ if (isIdentified())
+ return KeyTy(tag, name, file, line, scope, baseType, flags, sizeInBits,
+ alignInBits, elements, identifier);
+
+ return KeyTy(tag, name, file, line, scope, baseType, flags, sizeInBits,
+ alignInBits, elements, StringRef());
+ }
+
+ /// Compares two keys.
+ bool operator==(const KeyTy &other) const {
+ if (isIdentified())
+ // Just compare against the identifier.
+ return identifier == std::get<10>(other);
+
+ // Otherwise, compare the entire tuple.
+ return other == getAsKey();
+ }
+
+ /// Returns the hash value of the key.
+ static llvm::hash_code hashKey(const KeyTy &key) {
+ const auto &[tag, name, file, line, scope, baseType, flags, sizeInBits,
+ alignInBits, elements, identifier] = key;
+
+ if (!identifier.empty())
+ // The hash only consists of the unique identifier string.
+ return llvm::hash_value(identifier);
+
+ // Otherwise, everything else is included in the hash.
+ return llvm::hash_combine(tag, name, file, line, scope, baseType, flags,
+ sizeInBits, alignInBits, elements);
+ }
+
+ /// Constructs new storage for an attribute.
+ static DICompositeTypeAttrStorage *
+ construct(AttributeStorageAllocator &allocator, const KeyTy &key) {
+ auto [tag, name, file, line, scope, baseType, flags, sizeInBits,
+ alignInBits, elements, identifier] = key;
+ elements = allocator.copyInto(elements);
+ if (!identifier.empty()) {
+ identifier = allocator.copyInto(identifier);
+ return new (allocator.allocate<DICompositeTypeAttrStorage>())
+ DICompositeTypeAttrStorage(tag, name, file, line, scope, baseType,
+ flags, sizeInBits, alignInBits, elements,
+ identifier);
+ }
+ return new (allocator.allocate<DICompositeTypeAttrStorage>())
+ DICompositeTypeAttrStorage(tag, name, file, line, scope, baseType,
+ flags, sizeInBits, alignInBits, elements);
+ }
+
+ LogicalResult mutate(AttributeStorageAllocator &allocator,
+ const ArrayRef<DINodeAttr>& elements) {
+ // Replace the elements.
+ this->elements = allocator.copyInto(elements);
+ return success();
+ }
+
+private:
+ unsigned tag;
+ StringAttr name;
+ DIFileAttr file;
+ uint32_t line;
+ DIScopeAttr scope;
+ DITypeAttr baseType;
+ DIFlags flags;
+ uint64_t sizeInBits;
+ uint64_t alignInBits;
+ ArrayRef<DINodeAttr> elements;
+ StringRef identifier;
+};
+
+} // namespace detail
+} // namespace LLVM
+} // namespace mlir
+
+#endif // DIALECT_LLVMIR_IR_ATTRDETAIL_H
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
index e2342670508ce..c0c2c425d3c49 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
@@ -10,6 +10,8 @@
//
//===----------------------------------------------------------------------===//
+#include "AttrDetail.h"
+
#include "mlir/Dialect/LLVMIR/LLVMAttrs.h"
#include "mlir/Dialect/LLVMIR/LLVMDialect.h"
#include "mlir/IR/Builders.h"
@@ -123,7 +125,7 @@ bool MemoryEffectsAttr::isReadWrite() {
}
//===----------------------------------------------------------------------===//
-// DIExpression
+// DIExpressionAttr
//===----------------------------------------------------------------------===//
DIExpressionAttr DIExpressionAttr::get(MLIRContext *context) {
@@ -183,3 +185,408 @@ void printExpressionArg(AsmPrinter &printer, uint64_t opcode,
i++;
});
}
+
+//===----------------------------------------------------------------------===//
+// DICompositeTypeAttr
+//===----------------------------------------------------------------------===//
+
+DICompositeTypeAttr
+DICompositeTypeAttr::get(MLIRContext *context, unsigned tag, StringAttr name,
+ DIFileAttr file, uint32_t line, DIScopeAttr scope,
+ DITypeAttr baseType, DIFlags flags,
+ uint64_t sizeInBits, uint64_t alignInBits,
+ ::llvm::ArrayRef<DINodeAttr> elements) {
+ return Base::get(context, tag, name, file, line, scope, baseType, flags,
+ sizeInBits, alignInBits, elements, StringRef());
+}
+
+DICompositeTypeAttr DICompositeTypeAttr::get(
+ MLIRContext *context, StringRef identifier, unsigned tag, StringAttr name,
+ DIFileAttr file, uint32_t line, DIScopeAttr scope, DITypeAttr baseType,
+ DIFlags flags, uint64_t sizeInBits, uint64_t alignInBits,
+ ::llvm::ArrayRef<DINodeAttr> elements) {
+ return Base::get(context, tag, name, file, line, scope, baseType, flags,
+ sizeInBits, alignInBits, elements, identifier);
+}
+
+unsigned DICompositeTypeAttr::getTag() const { return getImpl()->getTag(); }
+
+StringAttr DICompositeTypeAttr::getName() const { return getImpl()->getName(); }
+
+DIFileAttr DICompositeTypeAttr::getFile() const { return getImpl()->getFile(); }
+
+uint32_t DICompositeTypeAttr::getLine() const { return getImpl()->getLine(); }
+
+DIScopeAttr DICompositeTypeAttr::getScope() const {
+ return getImpl()->getScope();
+}
+
+DITypeAttr DICompositeTypeAttr::getBaseType() const {
+ return getImpl()->getBaseType();
+}
+
+DIFlags DICompositeTypeAttr::getFlags() const { return getImpl()->getFlags(); }
+
+uint64_t DICompositeTypeAttr::getSizeInBits() const {
+ return getImpl()->getSizeInBits();
+}
+
+uint64_t DICompositeTypeAttr::getAlignInBits() const {
+ return getImpl()->getAlignInBits();
+}
+
+::llvm::ArrayRef<DINodeAttr> DICompositeTypeAttr::getElements() const {
+ return getImpl()->getElements();
+}
+
+StringRef DICompositeTypeAttr::getIdentifier() const {
+ return getImpl()->getIdentifier();
+}
+
+Attribute DICompositeTypeAttr::parse(AsmParser &parser, Type type) {
+ const Location loc = parser.getEncodedSourceLoc(parser.getCurrentLocation());
+ FailureOr<AsmParser::CyclicParseReset> cyclicParse;
+ FailureOr<unsigned> tag;
+ FailureOr<StringAttr> name;
+ FailureOr<DIFileAttr> file;
+ FailureOr<uint32_t> line;
+ FailureOr<DIScopeAttr> scope;
+ FailureOr<DITypeAttr> baseType;
+ FailureOr<DIFlags> flags;
+ FailureOr<uint64_t> sizeInBits;
+ FailureOr<uint64_t> alignInBits;
+ FailureOr<SmallVector<DINodeAttr>> elements;
+
+ std::string identifier;
+ bool skipFirstKeyword = false;
+
+ // Begin parsing.
+ if (parser.parseLess()) {
+ parser.emitError(parser.getCurrentLocation(), "expected `<`.");
+ return {};
+ }
+
+ auto paramParser = [&](const bool expectElements,
+ bool &hasElements) -> LogicalResult {
+ StringRef paramKey;
+
+ /// The first key word needs needs to skipped because it would have already
+ /// been parsed when attempting to parse the identifier.
+ if (skipFirstKeyword) {
+ // The parameter key is the identifier string.
+ paramKey = identifier;
+ skipFirstKeyword = false;
+ } else {
+ if (parser.parseKeyword(¶mKey)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "expected parameter name.");
+ }
+ }
+
+ if (parser.parseEqual()) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "expected `=` following parameter name.");
+ }
+
+ if (paramKey == "tag") {
+ tag = [&]() -> FailureOr<unsigned> {
+ StringRef nameKeyword;
+ if (parser.parseKeyword(&nameKeyword))
+ return failure();
+ if (const unsigned value = llvm::dwarf::getTag(nameKeyword))
+ return value;
+ return parser.emitError(parser.getCurrentLocation())
+ << "invalid debug info debug info tag name: " << nameKeyword;
+ }();
+ } else if (failed(name) && paramKey == "name") {
+ name = FieldParser<StringAttr>::parse(parser);
+ if (failed(name)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "failed to parse parameter 'name'");
+ }
+ } else if (failed(file) && paramKey == "file") {
+ file = FieldParser<DIFileAttr>::parse(parser);
+ if (failed(file)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "failed to parse parameter 'file'");
+ }
+ } else if (failed(line) && paramKey == "line") {
+ line = FieldParser<uint32_t>::parse(parser);
+ if (failed(line)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "failed to parse parameter 'line'");
+ }
+ } else if (failed(scope) && paramKey == "scope") {
+ scope = FieldParser<DIScopeAttr>::parse(parser);
+ if (failed(scope)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "failed to parse parameter 'scope'");
+ }
+ } else if (failed(baseType) && paramKey == "baseType") {
+ baseType = FieldParser<DITypeAttr>::parse(parser);
+ if (failed(baseType)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "failed to parse parameter 'baseType'");
+ }
+ } else if (failed(flags) && paramKey == "flags") {
+ flags = FieldParser<DIFlags>::parse(parser);
+ if (failed(flags)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "failed to parse parameter 'flags'");
+ }
+ } else if (failed(sizeInBits) && paramKey == "sizeInBits") {
+ sizeInBits = FieldParser<uint32_t>::parse(parser);
+ if (failed(sizeInBits)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "failed to parse parameter 'sizeInBits'");
+ }
+ } else if (failed(alignInBits) && paramKey == "alignInBits") {
+ alignInBits = FieldParser<uint32_t>::parse(parser);
+ if (failed(alignInBits)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "failed to parse parameter 'alignInBits'");
+ }
+ } else if (failed(elements) && paramKey == "elements") {
+ if (expectElements) {
+ if (parser.parseLSquare()) {
+ return parser.emitError(parser.getCurrentLocation(), "expected `[`.");
+ }
+
+ elements = FieldParser<SmallVector<DINodeAttr>>::parse(parser);
+ if (failed(elements)) {
+ return parser.emitError(parser.getCurrentLocation(),
+ "failed to parse parameter 'elements'");
+ }
+
+ if (parser.parseRSquare()) {
+ return parser.emitError(parser.getCurrentLocation(), "expected `]`.");
+ }
+ } else {
+ // Set hasElements to true to signal that parsing should cease until
+ // until after setting up recursive parsing.
+ hasElements = true;
+ return success();
+ }
+ } else {
+ return parser.emitError(parser.getCurrentLocation(), "unknown parameter '")
+ << paramKey << "'";
+ }
+ return success();
+ };
+
+ // This attribute is identified if a keyword followed by a comma or greater
+ // than is parsed.
+ if (succeeded(parser.parseOptionalKeywordOrString(&identifier))) {
+ bool hasElements = false;
+ skipFirstKeyword = true;
+ if (succeeded(parser.parseOptionalGreater()))
+ return getIdentified(parser.getContext(), identifier);
+
+ if (succeeded(parser.parseOptionalComma())) {
+ skipFirstKeyword = false;
+ // auto result = getChecked(
+ // [&] { return emitError(loc); }, loc.getContext(),
+ // StringRef(identifier));
+
+ // Parse immutable parameters.
+ do {
+ if (failed(paramParser(false, hasElements))) {
+ return {};
+ }
+
+ if (hasElements) {
+ // Stop parsing if "elements" was encountered. "elements" may be
+ // recursive in this context.
+ break;
+ }
+ } while (succeeded(parser.parseOptionalComma()));
+
+ if (succeeded(parser.parseOptionalGreater())) {
+ DICompositeTypeAttr result =
+ get(parser.getContext(), identifier, tag.value_or(0),
+ name.value_or(StringAttr()), file.value_or(DIFileAttr()),
+ line.value_or(0), scope.value_or(DIScopeAttr()),
+ baseType.value_or(DITypeAttr()), flags.value_or(DIFlags::Zero),
+ sizeInBits.value_or(0), alignInBits.value_or(0));
+
+ if (cyclicParse = parser.tryStartCyclicParse(result);
+ failed(cyclicParse)) {
+ parser.emitError(parser.getCurrentLocation(),
+ "only identifier allowed in recursive composite "
+ "type attribute.");
+ return {};
+ }
+ return result;
+ }
+ } else {
+ // Attempt to parse everything at once.
+ do {
+ if (failed(paramParser(true, hasElements))) {
+ return {};
+ }
+ } while (succeeded(parser.parseOptionalComma()));
+
+ // Expect the attribute to terminate.
+ if (parser.parseGreater()) {
+ parser.emitError(parser.getCurrentLocation(), "expected `>`.");
+ return {};
+ ...
[truncated]
|
waj334
force-pushed
the
implement-recursive-DICompositeTypeAttr
branch
from
ecaf007 to
64b34b2
Compare
You can test this locally with the following command:git-clang-format --diff 2ec95c19a267b5e58033701b12f55e0870bd3e6a fa557fc4265d93eb8c4b7ad943331a8201b1d083 -- mlir/lib/Dialect/LLVMIR/IR/AttrDetail.h mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h mlir/include/mlir/Target/LLVMIR/Dialect/All.h mlir/lib/AsmParser/AttributeParser.cpp mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp mlir/lib/IR/AsmPrinter.cpp mlir/lib/Interfaces/DataLayoutInterfaces.cpp mlir/lib/Target/LLVMIR/DebugTranslation.cppView the diff from clang-format here.diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h
index c38bf1c66bb..05b46129178 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h
@@ -24,7 +24,7 @@ namespace mlir {
namespace LLVM {
namespace detail {
- struct DICompositeTypeAttrStorage;
+struct DICompositeTypeAttrStorage;
} // namespace detail
/// This class represents the base attribute for all debug info attributes.
diff --git a/mlir/include/mlir/Target/LLVMIR/Dialect/All.h b/mlir/include/mlir/Target/LLVMIR/Dialect/All.h
index 8a9932ad36b..316170f839e 100644
--- a/mlir/include/mlir/Target/LLVMIR/Dialect/All.h
+++ b/mlir/include/mlir/Target/LLVMIR/Dialect/All.h
@@ -63,7 +63,7 @@ registerAllGPUToLLVMIRTranslations(DialectRegistry ®istry) {
registerLLVMDialectTranslation(registry);
registerNVVMDialectTranslation(registry);
registerROCDLDialectTranslation(registry);
- //registerSPIRVDialectTranslation(registry);
+ // registerSPIRVDialectTranslation(registry);
// Extension required for translating GPU offloading Ops.
gpu::registerOffloadingLLVMTranslationInterfaceExternalModels(registry);
diff --git a/mlir/lib/Dialect/LLVMIR/IR/AttrDetail.h b/mlir/lib/Dialect/LLVMIR/IR/AttrDetail.h
index ccecd540ab0..05e633ec00b 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/AttrDetail.h
+++ b/mlir/lib/Dialect/LLVMIR/IR/AttrDetail.h
@@ -53,9 +53,7 @@ struct DICompositeTypeAttrStorage : public ::mlir::AttributeStorage {
StringAttr getIdentifier() const { return identifier; }
/// Returns true if this attribute is identified.
- bool isIdentified() const {
- return !(!identifier);
- }
+ bool isIdentified() const { return !(!identifier); }
/// Returns the respective key for this attribute.
KeyTy getAsKey() const {
@@ -88,14 +86,14 @@ struct DICompositeTypeAttrStorage : public ::mlir::AttributeStorage {
// Otherwise, everything else is included in the hash.
return hash_combine(tag, name, file, line, scope, baseType, flags,
- sizeInBits, alignInBits, elements);
+ sizeInBits, alignInBits, elements);
}
/// Constructs new storage for an attribute.
static DICompositeTypeAttrStorage *
construct(AttributeStorageAllocator &allocator, const KeyTy &key) {
auto [tag, name, file, line, scope, baseType, flags, sizeInBits,
- alignInBits, elements, identifier] = key;
+ alignInBits, elements, identifier] = key;
elements = allocator.copyInto(elements);
if (identifier) {
return new (allocator.allocate<DICompositeTypeAttrStorage>())
@@ -109,7 +107,7 @@ struct DICompositeTypeAttrStorage : public ::mlir::AttributeStorage {
}
LogicalResult mutate(AttributeStorageAllocator &allocator,
- const ArrayRef<DINodeAttr>& elements) {
+ const ArrayRef<DINodeAttr> &elements) {
// Replace the elements.
this->elements = allocator.copyInto(elements);
return success();
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
index c11ed72fa35..97e92da0a6d 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
@@ -505,16 +505,11 @@ void DICompositeTypeAttr::print(AsmPrinter &printer) const {
if (getTag() > 0) {
valuePrinters.push_back(
- [&]() {
- printer << "tag = " << llvm::dwarf::TagString(getTag());
- });
+ [&]() { printer << "tag = " << llvm::dwarf::TagString(getTag()); });
}
if (getName()) {
valuePrinters.push_back([&]() {
-
-
-
printer << "name = ";
printer.printStrippedAttrOrType(getName());
});
diff --git a/mlir/lib/Interfaces/DataLayoutInterfaces.cpp b/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
index ce7f6371cb1..38487e44c3e 100644
--- a/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
+++ b/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
@@ -99,10 +99,9 @@ mlir::detail::getDefaultTypeSizeInBits(Type type, const DataLayout &dataLayout,
reportMissingDataLayout(type);
}
-template<typename T>
+template <typename T>
static DataLayoutEntryInterface
-findEntryForType(T type,
- ArrayRef<DataLayoutEntryInterface> params) {
+findEntryForType(T type, ArrayRef<DataLayoutEntryInterface> params) {
assert(!params.empty() && "expected non-empty parameter list");
std::map<unsigned, DataLayoutEntryInterface> sortedParams;
for (DataLayoutEntryInterface entry : params) {
diff --git a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
index 1f73d16b8ea..0414054f3c1 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
@@ -145,7 +145,7 @@ DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
attrToNode[attr] = result;
// Translate the elements.
- SmallVector<llvm::Metadata*> elements;
+ SmallVector<llvm::Metadata *> elements;
for (const DINodeAttr member : attr.getElements())
elements.push_back(translate(member));
|
waj334
changed the title
|
Thanks for tackling this problem! I have two initial comments:
I am not sure how easy it is to land 1) but it may be worth considering to revive the discussion (either on the PR or maybe better on discourse (https://discourse.llvm.org/t/rfc-supporting-aliases-in-cyclic-types-and-attributes/73236). |
For now I'll move on without #1 and I'll refactor if it lands between now and when this is in an acceptable state. As for printing and parsing, the only strange quirk I found was with elements needing to appear last in the comma separated list because I was trying to keep most of the old IR format while also making the other attributes "immutable". A side-effect would be that if any other attribute appears after elements, they would not get set because they need to be constructed with the id initially. I think I'm going to separate the elements from the rest of the attributes to make it more straightforward to parse. I'm thinking: llvm.di_composite_type<distinct[0]<>, other = ..., attr = ..., params = ... , .... (elements...) > where of course the id parameter can be omitted. I might have to make the other params mutable, but only allow it during parsing.
This is a reasonable (and easy) change. I'll replace the string id. |
waj334
force-pushed
the
implement-recursive-DICompositeTypeAttr
branch
from
64b34b2 to
e880caf
Compare
Regarding 1), just landing those PRs won't improve the parsing and printing code in its current state. This PR already nicely uses the APIs for doing cyclic printing that the alias statement infrastructure relies on, meaning no changes in the code of this PR would be required after the alias support would land. Improving the state of mutable attributes and types by adding support for ODS will require further work ontop of what is proposed in the RFC. Regarding implementing the RFC, I likely won't get to pushing on it until February. While I think the approach should be fine, there are implementation quality issues in the current PRs. I would therefore suggest to keep the approach here as is. |
Thanks for the update. I was mostly curios if there is any chance to rely on the alias printing. Let's continue with what we have then. I will try to review tomorrow or latest Tuesday. |
There was a problem hiding this comment.
Nice!
I did a first pass trying to remember how mutable attributes work and added some comments and questions. One thing that may simplifying the parsing and printing a bit is factoring a helper attribute that contains the immutable payload data. That way we have less handwritten code to maintain.
Regarding the format change you are planing I guess it makes sense that the elements need to be at the end of the attribute. I would probably still try to keep a similar syntax compared to the other attributes and just produce an error in the parser if the elements are not at the end?
llvm.di_composite_type<id=distinct[0]<>, other = ..., attr = ..., params = ... , ...., elements = ... >
However, if another syntax makes it more explicit that the elements need to be at the end that sounds fine as well.
*** edit ***
I thought a bit about it over lunch. I think you are right that it may make sense to clearly separate the elements from the immutable part of the attribute. Something like this would be very clear:
llvm.di_composite_type<id=distinct[0]<>, info = ... >[#type1, #llvm.di_composite_type<id=distinct[0]<>>]
I am unsure is that syntax works though or if it conflicts with some other construct.
| OptionalParameter<"uint64_t">:$sizeInBits, | ||
| OptionalParameter<"uint64_t">:$alignInBits, | ||
| OptionalArrayRefParameter<"DINodeAttr">:$elements, | ||
| OptionalParameter<"DistinctAttr">:$identifier |
There was a problem hiding this comment.
I think it makes sense to keep the identifier as a StringAttr since it actually has a counterpart in LLVM IR. At the same time, I would rename the DistinctAttr field to "id" or similar.
There was a problem hiding this comment.
...
DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
// TODO: Use distinct attributes to model this, once they have landed.
You have this TODO in DebugTranslation. What exactly was intended to be done with DistinctAttr? I'm chose to just make the unique key distinct, but seems to elude to the entire DICompositeTypeAttr being distinct.
There was a problem hiding this comment.
Adding a DistinctAttr as a field to the DICompositeType makes the entire Attribute distinct. It will never be uniqued with another DICompositeType since the 'íd' is always different. Does that make sense?
The idea of the comment was that a DICompositeType which has a DistinctAtt should map to a distinct metadata node, while all other DICompositeTypes would be mapped to normal metadata nodes that are uniqued like normal attributes.
| OptionalParameter<"unsigned">:$tag, | ||
| OptionalParameter<"StringAttr">:$name, | ||
| OptionalParameter<"DIFileAttr">:$file, | ||
| OptionalParameter<"uint32_t">:$line, | ||
| OptionalParameter<"DIScopeAttr">:$scope, | ||
| OptionalParameter<"DITypeAttr">:$baseType, | ||
| OptionalParameter<"DIFlags", "DIFlags::Zero">:$flags, | ||
| OptionalParameter<"uint64_t">:$sizeInBits, | ||
| OptionalParameter<"uint64_t">:$alignInBits, | ||
| OptionalArrayRefParameter<"DINodeAttr">:$elements, | ||
| OptionalParameter<"DistinctAttr">:$identifier |
There was a problem hiding this comment.
I would consider putting all the non-mutable fields (so probably everything except for the id and the elements) into a separate helper attribute that can be defined in tablegen. That way there printing an parsing code gets much simpler and we can more easily make parameters optional or mandatory and add new fields if necessary.
The new attribute could be called LLVM_DICompositeTypeInfoAttr or similar and it should not derive from any base class. Additionally, it probably makes sense to make sure it prints using an alias to make the DICompositType definitions a bit shorter.
I know that such attribute composition is not super nice either. However, I believe I prefer the slight inconveniences when using the attribute (for most compiler passes this is a route through thing anyways) over long and error prone printer and parser methods. Especially since I assume we may add new parameters here and make some of them non-optional etc.
| ]; | ||
| let extraClassDeclaration = [{ | ||
| static DICompositeTypeAttr getIdentified(MLIRContext *context, DistinctAttr identifier); | ||
| void replaceElements(const ArrayRef<DINodeAttr>& elements); |
There was a problem hiding this comment.
| void replaceElements(const ArrayRef<DINodeAttr>& elements); | |
| void replaceElements(ArrayRef<DINodeAttr> elements); |
nit: just ArrayRef should be sufficient since it is a constant reference on its own.
| //===- AttrDetail.h - Details of MLIR LLVM dialect attributes --------*- C++ | ||
| //-*-===// |
| //===----------------------------------------------------------------------===// | ||
| #ifndef DIALECT_LLVMIR_IR_ATTRDETAIL_H |
There was a problem hiding this comment.
| //===----------------------------------------------------------------------===// | |
| #ifndef DIALECT_LLVMIR_IR_ATTRDETAIL_H | |
| //===----------------------------------------------------------------------===// | |
| #ifndef DIALECT_LLVMIR_IR_ATTRDETAIL_H |
nit: newline
|
|
||
| /// Compares two keys. | ||
| bool operator==(const KeyTy &other) const { | ||
| if (isIdentified()) |
There was a problem hiding this comment.
I like you design choice of only using the distinct attribute for uniquing (when computing the has and the operation==) but returning everything with getKeyAs which is nice when walking the subelements of the attribute. Can you add some comments that explain these design choices for the next reviewer / next person modifying the code.
| } | ||
| } else { | ||
| return parser.emitError(parser.getCurrentLocation(), | ||
| "unknown parameter '") |
There was a problem hiding this comment.
If I understand correctly the error could be triggered as well if a parameter is parsed for the second time. So maybe say unknown parameter or parameter has been parsed before?
|
|
||
| // Short-circuit the mapping for this attribute to prevent infinite recursion | ||
| // if this composite type is encountered while translating the elements. | ||
| attrToNode[attr] = result; |
There was a problem hiding this comment.
Makes sense. A test with mutual recursive composite types would be nice to test this.
| if (const unsigned value = llvm::dwarf::getTag(nameKeyword)) | ||
| return value; | ||
| return parser.emitError(parser.getCurrentLocation()) | ||
| << "invalid debug info debug info tag name: " << nameKeyword; |
There was a problem hiding this comment.
| << "invalid debug info debug info tag name: " << nameKeyword; | |
| << "invalid debug info tag name: " << nameKeyword; |
| } | ||
|
|
||
| DICompositeTypeAttr | ||
| DICompositeTypeAttr::getIdentified(MLIRContext *context, |
There was a problem hiding this comment.
I guess this function is worth a comment that explains we use it to lookup an existing identified attribute and that is why we can pass in null values for anything except for the distinct attribute. I assume there is now way to assert here that the attribute actually has been created before with real payload? Or am I misunderstanding something?
|
@waj334 regarding the format I now saw that your suggestion comes from LLVM struct, which seems to put the mutable part in parenthesis? Let's follow their style then. It is always good to use existing stuff if there is precedence! Sorry didn't realize this yesterday. Also, I wonder the parameter names even need to be printed in front of the parameters since there are only three fields left (id, info, elements). At least, if you introduce a di_composite_type_info attribute. Up to you to choose what makes more sense! |
I think this may be causing the enumeration of aliases to become out of order. These aliases for the struct member attributes are not even used by anything because mutable attributes don't support them yet. Should these unused aliases even be outputted? |
You can try to disable the alias printing yes ( ). I assume this may help. Do you understand where exactly the problem is coming from. Do you see it for self recursive structs or do you have to introduce a mutally recursive struct to see the problem. |
If I can detect that an attribute has an usage in a mutable attribute, I can just have printAlias return a failed result, right? |
I would have suggested to completely disable the alias printing for these types. That can be done inside LLVMOpAsmDialectInterface by removing the corresponding attributes from the TypeSwitch. Changes inside printAlias should not be necessary. |
This fixed the issue. |
|
@waj334 Is this work still active? I'm adding generic support for recursive types (beyond composites) to DI, which would probably subsume this work. Thought I'd check here in case you were still working on it. We currently have a discussion thread here if you're interested in following up on that. Thanks! |
I'll be picking this up again soon as I will have a bit more free time to continue the work. What I have so far functions, but if you have an alternative design in mind I'll be interested in what you are planning since my work primarily targets composites. |
Implements mutable storage for DICompositeTypeAttr in order to allow for self-references in its elements array. When the "identifier" parameter set non-empty, only this string participates in the hash key, though the storage is implemented such that only the "elements" parameter is mutable. The module translator will now create the respective instance of llvm::DICompositeType without elements and then it will call "llvm::DICompositeType::replaceElements" to set the elements after each element is translated. The only required IR change was that elements are explicitly wrapped in square brackets for the sake of parsing.
waj334
force-pushed
the
implement-recursive-DICompositeTypeAttr
branch
from
e880caf to
fa557fc
Compare
|
@waj334 Just wanted to give you a heads up in case you haven't been following the discourse thread. I'm just about to send in #80251 which adds a recursive DI type interface that can be implemented by DI types that want to support recursion (only composite types for now). The main difference is it doesn't depend on mutable attrs, and can be used generically. Of course feel free to join the discussion thread if you feel strongly about mutable attrs. In the interest of time, I'll merge that implementation in first and we can always switch lanes down the road. Thanks! |
It seems I have a refactor in my future! I'll take a look at your PR when I get freed up again. |
Implements mutable storage for DICompositeTypeAttr in order to allow for self-references in its elements array. When the "identifier" parameter set non-empty, only this string participates in the hash key, though the storage is implemented such that only the "elements" parameter is mutable. The module translator will now create the respective instance of llvm::DICompositeType without elements and then it will call "llvm::DICompositeType::replaceElements" to set the elements after each element is translated. The only required IR change was that elements are explicitly wrapped in square brackets for the sake of parsing.