[CIR] Fix incorrect CIR_GlobalOp.global_visibility assembly format#189673
[CIR] Fix incorrect CIR_GlobalOp.global_visibility assembly format#189673andykaylor merged 2 commits intollvm:mainfrom
Conversation
Use custom parser for CIR_VisibilityAttr in cir.global like cir.func does to avoid issues with attribute being incorrectly printed and not parsed
NotLebedev
requested review from
andykaylor,
bcardosolopes,
lanza and
xlauko
as code owners
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-clang Author: Artemiy (NotLebedev) ChangesCloses #189666 . Fix incorrect printing and parsing of Resulted in keyword sticking to previous word and producing incorrect cir like this: Using custom parser/printer that is used in Also added tests for both global values and functions. Full diff: https://github.com/llvm/llvm-project/pull/189673.diff 4 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index b15dc30ffb87d..89652cb392bfb 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -2849,7 +2849,7 @@ def CIR_GlobalOp : CIR_Op<"global", [
let assemblyFormat = [{
($sym_visibility^)?
- (`` $global_visibility^)?
+ (custom<VisibilityAttr>($global_visibility)^)?
(`constant` $constant^)?
$linkage
(`comdat` $comdat^)?
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index eb322d135a804..1187fd5ddea5d 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -221,10 +221,16 @@ void printVisibilityAttr(OpAsmPrinter &printer,
}
}
-void parseVisibilityAttr(OpAsmParser &parser, cir::VisibilityAttr &visibility) {
+void printVisibilityAttr(OpAsmPrinter &printer, cir::GlobalOp,
+ cir::VisibilityAttr visibility) {
+ printVisibilityAttr(printer, visibility);
+}
+
+mlir::OptionalParseResult parseVisibilityAttr(OpAsmParser &parser, cir::VisibilityAttr &visibility) {
cir::VisibilityKind visibilityKind =
parseOptionalCIRKeyword(parser, cir::VisibilityKind::Default);
visibility = cir::VisibilityAttr::get(parser.getContext(), visibilityKind);
+ return mlir::success();
}
//===----------------------------------------------------------------------===//
diff --git a/clang/test/CIR/CodeGen/attribute-visibility.c b/clang/test/CIR/CodeGen/attribute-visibility.c
new file mode 100644
index 0000000000000..45098770d84ea
--- /dev/null
+++ b/clang/test/CIR/CodeGen/attribute-visibility.c
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t.cir
+// RUN: FileCheck --check-prefix=CIR --input-file=%t.cir %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o %t-cir.ll
+// RUN: FileCheck --check-prefix=LLVM --input-file=%t-cir.ll %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o %t.ll
+// RUN: FileCheck --check-prefix=OGCG --input-file=%t.ll %s
+
+int normal_var = 10;
+// CIR: cir.global external @normal_var {{.*}}
+// LLVM: @normal_var = global {{.*}}
+// OGCG: @normal_var = global {{.*}}
+
+__attribute__((visibility("hidden")))
+int hidden_var = 10;
+// CIR: cir.global hidden external @hidden_var {{.*}}
+// LLVM: @hidden_var = hidden global {{.*}}
+// OGCG: @hidden_var = hidden global {{.*}}
+
+static int normal_static_var = 10;
+// CIR: cir.global "private" internal dso_local @normal_static_var {{.*}}
+// LLVM: @normal_static_var = internal global {{.*}}
+// OGCG: @normal_static_var = internal global {{.*}}
+
+__attribute__((visibility("hidden")))
+static int hidden_static_var = 10;
+// CIR: cir.global "private" hidden internal dso_local @hidden_static_var {{.*}}
+// LLVM: @hidden_static_var = internal hidden global {{.*}}
+// OGCG: @hidden_static_var = internal global {{.*}}
+
+void normal_func() {
+ normal_var = 0;
+ normal_static_var = 0;
+}
+// CIR: cir.func no_inline no_proto dso_local @normal_func() {{.*}} {
+// LLVM: define dso_local void @normal_func() {{.*}}
+// OGCG: define dso_local void @normal_func() {{.*}}
+
+__attribute__((visibility("hidden")))
+void hidden_func() {
+ hidden_var = 0;
+ hidden_static_var = 0;
+}
+// CIR: cir.func no_inline no_proto hidden dso_local @hidden_func() {{.*}} {
+// LLVM: define hidden void @hidden_func() {{.*}}
+// OGCG: define hidden void @hidden_func() {{.*}}
diff --git a/clang/test/CIR/IR/attribute-visibility.cir b/clang/test/CIR/IR/attribute-visibility.cir
new file mode 100644
index 0000000000000..e4f09a9d657cf
--- /dev/null
+++ b/clang/test/CIR/IR/attribute-visibility.cir
@@ -0,0 +1,28 @@
+// RUN: cir-opt %s --verify-roundtrip | FileCheck %s
+
+!s32i = !cir.int<s, 32>
+module {
+ cir.global external dso_local @normal_var = #cir.int<10> : !s32i {alignment = 4 : i64}
+ // CHECK: cir.global external dso_local @normal_var = #cir.int<10> : !s32i {alignment = 4 : i64}
+
+ cir.global hidden external dso_local @hidden_var = #cir.int<10> : !s32i {alignment = 4 : i64}
+ // CHECK: cir.global hidden external dso_local @hidden_var = #cir.int<10> : !s32i {alignment = 4 : i64}
+
+ cir.global "private" internal dso_local @normal_static_var = #cir.int<10> : !s32i {alignment = 4 : i64}
+ // CHECK: cir.global "private" internal dso_local @normal_static_var = #cir.int<10> : !s32i {alignment = 4 : i64}
+
+ cir.global "private" hidden internal dso_local @hidden_static_var = #cir.int<10> : !s32i {alignment = 4 : i64}
+ // CHECK: cir.global "private" hidden internal dso_local @hidden_static_var = #cir.int<10> : !s32i {alignment = 4 : i64}
+
+ cir.func no_inline no_proto dso_local @normal_func() {
+ cir.return
+ }
+ // CHECK: cir.func no_inline no_proto dso_local @normal_func()
+ // CHECK: cir.return
+
+ cir.func no_inline no_proto hidden dso_local @hidden_func() {
+ cir.return
+ }
+ // CHECK: cir.func no_inline no_proto hidden dso_local @hidden_func()
+ // CHECK: cir.return
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
| let assemblyFormat = [{ | ||
| ($sym_visibility^)? | ||
| (`` $global_visibility^)? | ||
| (custom<VisibilityAttr>($global_visibility)^)? |
There was a problem hiding this comment.
| (custom<VisibilityAttr>($global_visibility)^)? | |
| ($global_visibility^)? |
Wouldn't this also fix the problem?
There was a problem hiding this comment.
Unfortunately it would not, because default parser breaks when VisibilityAttr default/optional attributes needs to be parsed.
It seems like it skips all optional arguments and goes straight to parsing linkage attribute and fails. But using cusotm parser works fine.
I looked into generated code and it seems that problem is the following:
- By default
Parser::parseOptionalAttributeis used when parsing optional attributes - It has four specializations here. For arrays, strings, symbol refs and default for generic
Attribute - Generic attribute accepts all kinds of
Token, but if it isbare_identifier(which I assume literalhiddenis) it tries to parse it as type withparseOptionalType(which it is not)
So straightforward fix here is to use custom parser in which case this code is generated:
if (auto optResult = [&]() -> ::mlir::OptionalParseResult {
{
auto odsResult = parseVisibilityAttr(parser, global_visibilityAttr);
if (!odsResult.has_value()) return {};
if (::mlir::failed(*odsResult)) return ::mlir::failure();
if (global_visibilityAttr)
result.getOrAddProperties<GlobalOp::Properties>().global_visibility = global_visibilityAttr;
}
return ::mlir::success();
}(); optResult.has_value() && ::mlir::failed(*optResult)) {
return ::mlir::failure();
} else if (optResult.has_value()) {
}
It does not use parseOptionalAttribute and uses parseVisibilityAttr directly. This is ultimately because CIR_VisibilityAttr uses custom assembly format
let assemblyFormat = [{
$value
}];
which does not work well with default implementation of parseOptionalType.
| // OGCG: @normal_static_var = internal global {{.*}} | ||
|
|
||
| __attribute__((visibility("hidden"))) | ||
| static int hidden_static_var = 10; |
There was a problem hiding this comment.
This is causing the test to crash when lowering to LLVM IR because we shouldn't be setting the hidden visibility for variables with local linkage. The CIR code to set visibility needs a general update to match classic codegen more closely. I'll do that in a separate PR. For now, you can just remove this variable from the test.
There was a problem hiding this comment.
Removed variable from test.
| cir.global "private" internal dso_local @normal_static_var = #cir.int<10> : !s32i {alignment = 4 : i64} | ||
| // CHECK: cir.global "private" internal dso_local @normal_static_var = #cir.int<10> : !s32i {alignment = 4 : i64} | ||
|
|
||
| cir.global "private" hidden internal dso_local @hidden_static_var = #cir.int<10> : !s32i {alignment = 4 : i64} |
There was a problem hiding this comment.
The verifier should reject this because it won't lower to LLVM IR correctly. That can happen in a follow-up change though.
|
@NotLebedev Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
3 participants
Closes #189666 .
Fix incorrect printing and parsing of
cir.globalifglobal_visibilityattribute is present. Incorrect assembly formatResulted in keyword sticking to previous word and producing incorrect cir like this:
Using custom parser/printer that is used in
cir.funcparser fixes this issue and makes printed/parsed attribute for functions and global values consistent.Also added tests for both global values and functions.