[clang][TableGen] Fix Duplicate Entries in TableGen #140828

Mr-Anyone · 2025-05-21T01:32:35Z

Fixed TableGen duplicate issues that causes the wrong interrupt attribute from being selected.

resolves #140701

llvmbot · 2025-05-21T01:33:13Z

@llvm/pr-subscribers-backend-risc-v

Author: Vincent (Mr-Anyone)

Changes

Fixed TableGen duplicate issues that causes the wrong interrupt attribute from being selected.

resolves #140701

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

2 Files Affected:

(added) clang/test/AST/ast-dump-riscv-attributes.cpp (+12)
(modified) clang/utils/TableGen/ClangAttrEmitter.cpp (+21-5)

diff --git a/clang/test/AST/ast-dump-riscv-attributes.cpp b/clang/test/AST/ast-dump-riscv-attributes.cpp
new file mode 100644
index 0000000000000..7efe626072311
--- /dev/null
+++ b/clang/test/AST/ast-dump-riscv-attributes.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple riscv64 -ast-dump -ast-dump-filter c23 -std=c23 -x c %s | FileCheck --strict-whitespace %s
+
+// CHECK:       FunctionDecl{{.*}}pre_c23
+// CHECK-NEXT:    |-CompoundStmt
+// CHECK-NEXT:    `-RISCVInterruptAttr{{.*}}supervisor
+__attribute__((interrupt("supervisor"))) void pre_c23(){}
+
+// CHECK:       FunctionDecl{{.*}}in_c23
+// CHECK-NEXT:    |-CompoundStmt
+// CHECK-NEXT:    `-RISCVInterruptAttr{{.*}}supervisor
+// CHECK-NOT:     `-RISCVInterruptAttr{{.*}}machine
+[[gnu::interrupt("supervisor")]] void in_c23(){}
diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp
index 9684ec9520e5a..effef13c8d276 100644
--- a/clang/utils/TableGen/ClangAttrEmitter.cpp
+++ b/clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -20,6 +20,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -3667,6 +3668,9 @@ static bool GenerateTargetSpecificAttrChecks(const Record *R,
 static void GenerateHasAttrSpellingStringSwitch(
     ArrayRef<std::pair<const Record *, FlattenedSpelling>> Attrs,
     raw_ostream &OS, StringRef Variety, StringRef Scope = "") {
+
+  llvm::StringMap<std::string> TargetSpecificMap;
+
   for (const auto &[Attr, Spelling] : Attrs) {
     // C++11-style attributes have specific version information associated with
     // them. If the attribute has no scope, the version information must not
@@ -3727,12 +3731,24 @@ static void GenerateHasAttrSpellingStringSwitch(
       }
     }
 
-    std::string TestStr = !Test.empty()
-                              ? Test + " ? " + itostr(Version) + " : 0"
-                              : itostr(Version);
-    if (Scope.empty() || Scope == Spelling.nameSpace())
-      OS << "    .Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
+    std::string TestStr =
+        !Test.empty() ? '(' + Test + " ? " + itostr(Version) + " : 0" + ')'
+                      : '(' + itostr(Version) + ')';
+
+    if (Scope.empty() || Scope == Spelling.nameSpace()) {
+      if (TargetSpecificMap.contains(Spelling.name())) {
+        TargetSpecificMap[Spelling.name()] += " || " + TestStr;
+      } else {
+        TargetSpecificMap[Spelling.name()] = TestStr;
+      }
+    }
+  }
+
+  for (auto &entry : TargetSpecificMap) {
+    OS << "    .Case(\"" << entry.getKey() << "\", " << entry.getValue()
+       << ")\n";
   }
+
   OS << "    .Default(0);\n";
 }

llvmbot · 2025-05-21T01:33:13Z

@llvm/pr-subscribers-clang

Author: Vincent (Mr-Anyone)

Changes

Fixed TableGen duplicate issues that causes the wrong interrupt attribute from being selected.

resolves #140701

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

2 Files Affected:

(added) clang/test/AST/ast-dump-riscv-attributes.cpp (+12)
(modified) clang/utils/TableGen/ClangAttrEmitter.cpp (+21-5)

diff --git a/clang/test/AST/ast-dump-riscv-attributes.cpp b/clang/test/AST/ast-dump-riscv-attributes.cpp
new file mode 100644
index 0000000000000..7efe626072311
--- /dev/null
+++ b/clang/test/AST/ast-dump-riscv-attributes.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple riscv64 -ast-dump -ast-dump-filter c23 -std=c23 -x c %s | FileCheck --strict-whitespace %s
+
+// CHECK:       FunctionDecl{{.*}}pre_c23
+// CHECK-NEXT:    |-CompoundStmt
+// CHECK-NEXT:    `-RISCVInterruptAttr{{.*}}supervisor
+__attribute__((interrupt("supervisor"))) void pre_c23(){}
+
+// CHECK:       FunctionDecl{{.*}}in_c23
+// CHECK-NEXT:    |-CompoundStmt
+// CHECK-NEXT:    `-RISCVInterruptAttr{{.*}}supervisor
+// CHECK-NOT:     `-RISCVInterruptAttr{{.*}}machine
+[[gnu::interrupt("supervisor")]] void in_c23(){}
diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp
index 9684ec9520e5a..effef13c8d276 100644
--- a/clang/utils/TableGen/ClangAttrEmitter.cpp
+++ b/clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -20,6 +20,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -3667,6 +3668,9 @@ static bool GenerateTargetSpecificAttrChecks(const Record *R,
 static void GenerateHasAttrSpellingStringSwitch(
     ArrayRef<std::pair<const Record *, FlattenedSpelling>> Attrs,
     raw_ostream &OS, StringRef Variety, StringRef Scope = "") {
+
+  llvm::StringMap<std::string> TargetSpecificMap;
+
   for (const auto &[Attr, Spelling] : Attrs) {
     // C++11-style attributes have specific version information associated with
     // them. If the attribute has no scope, the version information must not
@@ -3727,12 +3731,24 @@ static void GenerateHasAttrSpellingStringSwitch(
       }
     }
 
-    std::string TestStr = !Test.empty()
-                              ? Test + " ? " + itostr(Version) + " : 0"
-                              : itostr(Version);
-    if (Scope.empty() || Scope == Spelling.nameSpace())
-      OS << "    .Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
+    std::string TestStr =
+        !Test.empty() ? '(' + Test + " ? " + itostr(Version) + " : 0" + ')'
+                      : '(' + itostr(Version) + ')';
+
+    if (Scope.empty() || Scope == Spelling.nameSpace()) {
+      if (TargetSpecificMap.contains(Spelling.name())) {
+        TargetSpecificMap[Spelling.name()] += " || " + TestStr;
+      } else {
+        TargetSpecificMap[Spelling.name()] = TestStr;
+      }
+    }
+  }
+
+  for (auto &entry : TargetSpecificMap) {
+    OS << "    .Case(\"" << entry.getKey() << "\", " << entry.getValue()
+       << ")\n";
   }
+
   OS << "    .Default(0);\n";
 }

Fixed TableGen duplicate issues that causes the wrong interrupt attribute from being selected. resolves llvm#140701

github-actions · 2025-05-21T02:21:35Z

✅ With the latest revision this PR passed the C/C++ code formatter.

AaronBallman

Thank you for working on this! In general, this looks good to me, just nits about our coding style.

AaronBallman · 2025-05-21T11:57:19Z

clang/utils/TableGen/ClangAttrEmitter.cpp

+  }
+
+  // Create the actual string switch statement after all the attributes have
+  // been parsed


Suggested change

// been parsed

// been parsed.

AaronBallman · 2025-05-21T11:57:40Z

clang/utils/TableGen/ClangAttrEmitter.cpp

+  for (auto &entry : TestStringMap) {
+    OS << "    .Case(\"" << entry.getKey() << "\", " << entry.getValue()
+       << ")\n";


Suggested change

for (auto &entry : TestStringMap) {

OS << " .Case(\"" << entry.getKey() << "\", " << entry.getValue()

<< ")\n";

for (auto &Entry : TestStringMap) {

OS << " .Case(\"" << Entry.getKey() << "\", " << Entry.getValue()

<< ")\n";

AaronBallman · 2025-05-21T11:57:58Z

clang/utils/TableGen/ClangAttrEmitter.cpp

+      if (TestStringMap.contains(Spelling.name())) {
+        TestStringMap[Spelling.name()] += " || " + TestStr;
+      } else {
+        TestStringMap[Spelling.name()] = TestStr;
+      }


Suggested change

if (TestStringMap.contains(Spelling.name())) {

TestStringMap[Spelling.name()] += " || " + TestStr;

} else {

TestStringMap[Spelling.name()] = TestStr;

}

if (TestStringMap.contains(Spelling.name()))

TestStringMap[Spelling.name()] += " || " + TestStr;

else

TestStringMap[Spelling.name()] = TestStr;

done, thanks.

erichkeane · 2025-05-21T12:31:06Z

clang/test/AST/ast-dump-riscv-attributes.cpp

+// CHECK:       FunctionDecl{{.*}}in_c23
+// CHECK-NEXT:    |-CompoundStmt
+// CHECK-NEXT:    `-RISCVInterruptAttr{{.*}}supervisor
+// CHECK-NOT:     `-RISCVInterruptAttr{{.*}}machine


I don't get this test. Why does this choose the RISCV interrupt instead of the GNU one? And where would 'machine' come from?

"-triple riscv64" is why you get the RISCVInterruptAttr, which is expected.

"machine" is on a CHECK-NOT line, which is what the bug was about.

I guess the question is 'how'? Is the "supervisor" not coming from the string?

Before this patch, our "has attribute" check was looking through a StringSwitch that had multiple cases with the same string argument but different resulting values (that would check for target triples and arches). So we'd hit the first case, say "this matches", and the test for the attribute would only pass if the target triple matched. Because the first triple was never the RISCV attribute, we'd always say "we don't have this attribute". In turn, this would mean parsing would say "well, we don't know about this attribute, so eat all the arguments". But because Sema could figure out which handleInterruptAttr function to call, we'd actually go ahead and make the correct attribute, just with the incorrect argument. The default if no argument is specified for RISCV is machine. So we'd ignore supervisor and just substitute in machine quietly for the user.

erichkeane · 2025-05-21T12:31:50Z

clang/utils/TableGen/ClangAttrEmitter.cpp

-                              : itostr(Version);
-    if (Scope.empty() || Scope == Spelling.nameSpace())
-      OS << "    .Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
+    std::string TestStr =


Can you share a before/after generated file so that I can take a look? I'm having a hard time visualizing what this does.

Before: https://pastebin.com/iwkWd3Bf
After: https://pastebin.com/eikjMpQt

Take the interrupt attribute as an example:

This is the original string: true && (T.getArch() == llvm::Triple::riscv32 || T.getArch() == llvm::Triple::riscv64) ? 1 : 0.

The problem is that there are multiple of them for a single attribute, such as true && (T.getArch() == llvm::Triple::arm || T.getArch() == llvm::Triple::thumb || T.getArch() == llvm::Triple::armeb || T.getArch() == llvm::Triple::thumbeb) ? 1 : 0 for arm.

Meaning that, we used to get:

.Case("interrupt",true && (T.getArch() == llvm::Triple::riscv32 || T.getArch() == llvm::Triple::riscv64) ? 1 : 0 ) .Case("interrupt",true && (T.getArch() == llvm::Triple::arm || T.getArch() == llvm::Triple::thumb || T.getArch() == llvm::Triple::armeb || T.getArch() == llvm::Triple::thumbeb) ? 1 : 0 ...

This is the new one TestStr: (true && (T.getArch() == llvm::Triple::avr) ? 1 : 0), with the only difference being the added parentheses. We can join test strings together with ||, meaning we generate something like this:

(true && (T.getArch() == llvm::Triple::riscv32 || T.getArch() == llvm::Triple::riscv64) ? 1 : 0) || (true && (T.getArch() == llvm::Triple::arm || T.getArch() == llvm::Triple::thumb || T.getArch() == llvm::Triple::armeb || T.getArch() == llvm::Triple::thumbeb) ? 1 : 0) ...

So now we have a single case statement instead,

.Case("interrupt",(true && (T.getArch() == llvm::Triple::riscv32 || T.getArch() == llvm::Triple::riscv64) ? 1 : 0) || (true && (T.getArch() == llvm::Triple::arm || T.getArch() == llvm::Triple::thumb || T.getArch() == llvm::Triple::armeb || T.getArch() == llvm::Triple::thumbeb) ? 1 : 0) ... ) ...

Got it, thank you for that!

AaronBallman

LGTM!

llvmbot added clang Clang issues not falling into any other category backend:RISC-V labels May 21, 2025

Mr-Anyone force-pushed the attr_fix branch from 640bc8b to f9b4429 Compare May 21, 2025 01:37

[clang][TableGen] Fix Duplicate Entries in TableGen

393ccc1

Fixed TableGen duplicate issues that causes the wrong interrupt attribute from being selected. resolves llvm#140701

Mr-Anyone force-pushed the attr_fix branch from f9b4429 to 393ccc1 Compare May 21, 2025 02:15

update comments

475f240

Fixed Clang Format

7a5e56c

topperc requested a review from AaronBallman May 21, 2025 03:38

Added Release Docs

adaf399

AaronBallman requested a review from erichkeane May 21, 2025 11:56

AaronBallman added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label May 21, 2025

AaronBallman reviewed May 21, 2025

View reviewed changes

erichkeane reviewed May 21, 2025

View reviewed changes

Mr-Anyone force-pushed the attr_fix branch from 98d0c69 to 50a03e7 Compare May 21, 2025 15:06

Minor nits

df54036

Mr-Anyone force-pushed the attr_fix branch from 50a03e7 to df54036 Compare May 21, 2025 15:15

Mr-Anyone requested review from AaronBallman and erichkeane May 21, 2025 15:18

erichkeane approved these changes May 21, 2025

View reviewed changes

AaronBallman approved these changes May 21, 2025

View reviewed changes

AaronBallman merged commit 385752c into llvm:main May 21, 2025
10 of 11 checks passed

[clang][TableGen] Fix Duplicate Entries in TableGen #140828

[clang][TableGen] Fix Duplicate Entries in TableGen #140828

Uh oh!

Conversation

Mr-Anyone commented May 21, 2025

Uh oh!

llvmbot commented May 21, 2025

Uh oh!

llvmbot commented May 21, 2025

Uh oh!

github-actions bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mr-Anyone May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented May 21, 2025 •

edited

Loading

Mr-Anyone May 21, 2025 •

edited

Loading