[Clang][TableGen] Add Features to TargetBuiltin

[wangpc-pp]

RISCV target will use this parameter, so we need a way to specify
it.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Wang Pengcheng (wangpc-pp)

Changes

RISCV target will use this parameter, so we need a way to specify
it.

---

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

2 Files Affected:

• (modified) clang/include/clang/Basic/BuiltinsBase.td (+3-1)
• (modified) clang/utils/TableGen/ClangBuiltinsEmitter.cpp (+1-1)

diff --git a/clang/include/clang/Basic/BuiltinsBase.td b/clang/include/clang/Basic/BuiltinsBase.td
index b65b41be03265..bfccff5600ddb 100644
--- a/clang/include/clang/Basic/BuiltinsBase.td
+++ b/clang/include/clang/Basic/BuiltinsBase.td
@@ -87,7 +87,9 @@ class CustomEntry {
 }
 
 class AtomicBuiltin : Builtin;
-class TargetBuiltin : Builtin;
+class TargetBuiltin : Builtin {
+  string Features = "";
+}
 
 class LibBuiltin<string header, string languages = "ALL_LANGUAGES"> : Builtin {
   string Header = header;
diff --git a/clang/utils/TableGen/ClangBuiltinsEmitter.cpp b/clang/utils/TableGen/ClangBuiltinsEmitter.cpp
index dc10fa14c5959..48f55b8af97e4 100644
--- a/clang/utils/TableGen/ClangBuiltinsEmitter.cpp
+++ b/clang/utils/TableGen/ClangBuiltinsEmitter.cpp
@@ -219,7 +219,7 @@ void EmitBuiltinDef(llvm::raw_ostream &OS, StringRef Substitution,
     break;
   }
   case BuiltinType::TargetBuiltin:
-    OS << ", \"\"";
+    OS << ", \"" << Builtin->getValueAsString("Features") << "\"";
     break;
   case BuiltinType::AtomicBuiltin:
   case BuiltinType::Builtin:

[wangpc-pp]

Ping.

[AaronBallman]

The changes seem reasonable to me but I'd feel more comfortable if the functionality was also being used (so that we'd get test coverage verifying its correctness). Do you think it would be reasonable to include the RISCV changes as well?

[wangpc-pp]

The changes seem reasonable to me but I'd feel more comfortable if the functionality was also being used (so that we'd get test coverage verifying its correctness). Do you think it would be reasonable to include the RISCV changes as well?

Yeah, I separated RISCV changes into another PR(#80280).

(I used SPR to do the stacked patches, but it seems that it won't refer to its parents or children)

[AaronBallman]

The changes seem reasonable to me but I'd feel more comfortable if the functionality was also being used (so that we'd get test coverage verifying its correctness). Do you think it would be reasonable to include the RISCV changes as well?

Yeah, I separated RISCV changes into another PR(#80280).

(I used SPR to do the stacked patches, but it seems that it won't refer to its parents or children)

Ah, yeah, I had no idea this was part of a stack of patches. :-( I think this is fine as-is now, thank you!

[AaronBallman]

LGTM