-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[mlir][LLVMIR] Handle anonymous TBAA roots during metadata emission #169167
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
Conversation
|
@llvm/pr-subscribers-mlir-llvm Author: Men-cotton (Men-cotton) ChangesFixes: #160721 Now Full diff: https://github.com/llvm/llvm-project/pull/169167.diff 2 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index caecc0f774155..412a5f76d5753 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -677,10 +677,10 @@ llvm::Constant *mlir::LLVM::detail::getLLVMConstant(
}
}
}
- // std::vector is used here to accomodate large number of elements that
- // exceed SmallVector capacity.
- std::vector<llvm::Constant *> constants(numElements, child);
- return llvm::ConstantArray::get(arrayType, constants);
+ // std::vector is used here to accomodate large number of elements that
+ // exceed SmallVector capacity.
+ std::vector<llvm::Constant *> constants(numElements, child);
+ return llvm::ConstantArray::get(arrayType, constants);
}
}
@@ -2131,8 +2131,16 @@ LogicalResult ModuleTranslation::createTBAAMetadata() {
// LLVM metadata instances.
AttrTypeWalker walker;
walker.addWalk([&](TBAARootAttr root) {
- tbaaMetadataMapping.insert(
- {root, llvm::MDNode::get(ctx, llvm::MDString::get(ctx, root.getId()))});
+ llvm::MDNode *node;
+ if (StringAttr id = root.getId()) {
+ node = llvm::MDNode::get(ctx, llvm::MDString::get(ctx, id));
+ } else {
+ // Anonymous root nodes are self-referencing.
+ auto selfRef = llvm::MDNode::getTemporary(ctx, {});
+ node = llvm::MDNode::get(ctx, {selfRef.get()});
+ node->replaceOperandWith(0, node);
+ }
+ tbaaMetadataMapping.insert({root, node});
});
walker.addWalk([&](TBAATypeDescriptorAttr descriptor) {
diff --git a/mlir/test/Target/LLVMIR/anonymous-tbaa.mlir b/mlir/test/Target/LLVMIR/anonymous-tbaa.mlir
new file mode 100644
index 0000000000000..b54bfe47e0807
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/anonymous-tbaa.mlir
@@ -0,0 +1,21 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+#tbaa_root_0 = #llvm.tbaa_root<>
+#tbaa_type_desc_1 = #llvm.tbaa_type_desc<id = "omnipotent char", members = {<#tbaa_root_0, 0>}>
+#tbaa_type_desc_2 = #llvm.tbaa_type_desc<id = "long long", members = {<#tbaa_type_desc_1, 0>}>
+#tbaa_tag_3 = #llvm.tbaa_tag<access_type = #tbaa_type_desc_2, base_type = #tbaa_type_desc_2, offset = 0>
+
+// CHECK: define void @tbaa_anonymous_root(ptr %{{.*}}) {
+// CHECK: %{{.*}} = load i64, ptr %{{.*}}, align 4, !tbaa ![[TAG:[0-9]+]]
+// CHECK: ret void
+// CHECK: }
+// CHECK: !llvm.module.flags = !{![[FLAGS:[0-9]+]]}
+// CHECK: ![[FLAGS]] = !{i32 2, !"Debug Info Version", i32 3}
+// CHECK: ![[TAG]] = !{![[TYPE:[0-9]+]], ![[TYPE]], i64 0}
+// CHECK: ![[TYPE]] = !{!"long long", ![[BASE:[0-9]+]], i64 0}
+// CHECK: ![[BASE]] = !{!"omnipotent char", ![[ROOT:[0-9]+]], i64 0}
+// CHECK: ![[ROOT]] = distinct !{![[ROOT]]}
+llvm.func @tbaa_anonymous_root(%arg0: !llvm.ptr) {
+ %0 = llvm.load %arg0 {tbaa = [#tbaa_tag_3]} : !llvm.ptr -> i64
+ llvm.return
+}
|
|
@llvm/pr-subscribers-mlir Author: Men-cotton (Men-cotton) ChangesFixes: #160721 Now Full diff: https://github.com/llvm/llvm-project/pull/169167.diff 2 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index caecc0f774155..412a5f76d5753 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -677,10 +677,10 @@ llvm::Constant *mlir::LLVM::detail::getLLVMConstant(
}
}
}
- // std::vector is used here to accomodate large number of elements that
- // exceed SmallVector capacity.
- std::vector<llvm::Constant *> constants(numElements, child);
- return llvm::ConstantArray::get(arrayType, constants);
+ // std::vector is used here to accomodate large number of elements that
+ // exceed SmallVector capacity.
+ std::vector<llvm::Constant *> constants(numElements, child);
+ return llvm::ConstantArray::get(arrayType, constants);
}
}
@@ -2131,8 +2131,16 @@ LogicalResult ModuleTranslation::createTBAAMetadata() {
// LLVM metadata instances.
AttrTypeWalker walker;
walker.addWalk([&](TBAARootAttr root) {
- tbaaMetadataMapping.insert(
- {root, llvm::MDNode::get(ctx, llvm::MDString::get(ctx, root.getId()))});
+ llvm::MDNode *node;
+ if (StringAttr id = root.getId()) {
+ node = llvm::MDNode::get(ctx, llvm::MDString::get(ctx, id));
+ } else {
+ // Anonymous root nodes are self-referencing.
+ auto selfRef = llvm::MDNode::getTemporary(ctx, {});
+ node = llvm::MDNode::get(ctx, {selfRef.get()});
+ node->replaceOperandWith(0, node);
+ }
+ tbaaMetadataMapping.insert({root, node});
});
walker.addWalk([&](TBAATypeDescriptorAttr descriptor) {
diff --git a/mlir/test/Target/LLVMIR/anonymous-tbaa.mlir b/mlir/test/Target/LLVMIR/anonymous-tbaa.mlir
new file mode 100644
index 0000000000000..b54bfe47e0807
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/anonymous-tbaa.mlir
@@ -0,0 +1,21 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+#tbaa_root_0 = #llvm.tbaa_root<>
+#tbaa_type_desc_1 = #llvm.tbaa_type_desc<id = "omnipotent char", members = {<#tbaa_root_0, 0>}>
+#tbaa_type_desc_2 = #llvm.tbaa_type_desc<id = "long long", members = {<#tbaa_type_desc_1, 0>}>
+#tbaa_tag_3 = #llvm.tbaa_tag<access_type = #tbaa_type_desc_2, base_type = #tbaa_type_desc_2, offset = 0>
+
+// CHECK: define void @tbaa_anonymous_root(ptr %{{.*}}) {
+// CHECK: %{{.*}} = load i64, ptr %{{.*}}, align 4, !tbaa ![[TAG:[0-9]+]]
+// CHECK: ret void
+// CHECK: }
+// CHECK: !llvm.module.flags = !{![[FLAGS:[0-9]+]]}
+// CHECK: ![[FLAGS]] = !{i32 2, !"Debug Info Version", i32 3}
+// CHECK: ![[TAG]] = !{![[TYPE:[0-9]+]], ![[TYPE]], i64 0}
+// CHECK: ![[TYPE]] = !{!"long long", ![[BASE:[0-9]+]], i64 0}
+// CHECK: ![[BASE]] = !{!"omnipotent char", ![[ROOT:[0-9]+]], i64 0}
+// CHECK: ![[ROOT]] = distinct !{![[ROOT]]}
+llvm.func @tbaa_anonymous_root(%arg0: !llvm.ptr) {
+ %0 = llvm.load %arg0 {tbaa = [#tbaa_tag_3]} : !llvm.ptr -> i64
+ llvm.return
+}
|
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.
LGTM, thanks for the fix.
Let me know if you need help to land this.
gysit
left a comment
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.
Are you sure an anonymous TBAA root is represent with a self referencing node?
The language reference seems to hint that an empty node should be used:
https://llvm.org/docs/LangRef.html#representation
Or am I misreading something?
|
Thanks for reviewing, @gysit ! You are right about the LangRef description. However, I noticed that My implementation matches that pattern to be consistent with how LLVM typically builds anonymous roots. That said, if you prefer to strictly follow the LangRef text (using |
Ah, if the actual implementation differs from the language reference then the implementation takes preference. |
gysit
left a comment
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.
Thanks for clarifying!
LGTM
|
I don't have write access. Could you please merge this? |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/17432 Here is the relevant piece of the build log for the reference |
|
At least, I found that the same failure was reported in the pull request #167767. |
|
I've been looking into the post-merge CI failure. It appears to be a pre-existing issue in the Here is a summary of my investigation:
Given that my patch only touches MLIR, it seems highly unlikely to be the root cause of this JITLink test failure. It appears to be an intermittent issue that was present before my patch landed. |
|
Yeah, this does not look related in any way. |
…lvm#169167) This commit enhances MLIR's TBAA export with support for anonymous TBAA roots. The import for this was around for a bit but the export was missing. Fixes: llvm#160721
Fixes: #160721
Now
build/bin/mlir-translate -mlir-to-llvmir mlir/test/Dialect/LLVMIR/tbaa-roundtrip.mlirpasses.