Skip to content

[Clang][NFC] Refactor Targets.h to make it publicly accessible #116090

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

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

seven-mile
Copy link
Contributor

This PR is motivated by the requirements of ClangIR, which includes compilation pipelines that do not always start from the Clang driver. In these cases, accessing some target-specific information, such as obtaining a data layout string for a given target triple or querying other target details, requires foundational infrastructure like clang::TargetInfo. Since ClangIR is actively being upstreamed, sharing this logic across components has become essential, which leads to this PR.

The function clang::targets::AllocateTarget serves as the factory for Clang's TargetInfo. To enable sharing, this PR moves AllocateTarget to a public header.

The existing header clang/lib/Basic/Targets.h previously contained two parts: the AllocateTarget function and target-specific macro helpers. With AllocateTarget moved, only the macro stuff remain in Targets.h. To better organize the code, the macro helpers have been relocated to a new file, clang/lib/Basic/TargetDefines.h (essentially a rename). The original Targets.h now serves as a proxy header that includes both headers to maintain compatibility.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 13, 2024

@llvm/pr-subscribers-clang

Author: 7mile (seven-mile)

Changes

This PR is motivated by the requirements of ClangIR, which includes compilation pipelines that do not always start from the Clang driver. In these cases, accessing some target-specific information, such as obtaining a data layout string for a given target triple or querying other target details, requires foundational infrastructure like clang::TargetInfo. Since ClangIR is actively being upstreamed, sharing this logic across components has become essential, which leads to this PR.

The function clang::targets::AllocateTarget serves as the factory for Clang's TargetInfo. To enable sharing, this PR moves AllocateTarget to a public header.

The existing header clang/lib/Basic/Targets.h previously contained two parts: the AllocateTarget function and target-specific macro helpers. With AllocateTarget moved, only the macro stuff remain in Targets.h. To better organize the code, the macro helpers have been relocated to a new file, clang/lib/Basic/TargetDefines.h (essentially a rename). The original Targets.h now serves as a proxy header that includes both headers to maintain compatibility.


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

3 Files Affected:

  • (modified) clang/include/clang/Basic/TargetInfo.h (+5)
  • (added) clang/lib/Basic/TargetDefines.h (+39)
  • (modified) clang/lib/Basic/Targets.h (+1-26)
diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 25eda907d20a7b..71873ee2ca7cb9 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -1860,6 +1860,11 @@ class TargetInfo : public TransferrableTargetInfo,
   void CheckFixedPointBits() const;
 };
 
+namespace targets {
+std::unique_ptr<clang::TargetInfo>
+AllocateTarget(const llvm::Triple &Triple, const clang::TargetOptions &Opts);
+} // namespace targets
+
 }  // end namespace clang
 
 #endif
diff --git a/clang/lib/Basic/TargetDefines.h b/clang/lib/Basic/TargetDefines.h
new file mode 100644
index 00000000000000..01db32f6a3d710
--- /dev/null
+++ b/clang/lib/Basic/TargetDefines.h
@@ -0,0 +1,39 @@
+//===------- TargetDefines.h - Target define helpers ------------*- 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 declares a series of helper functions for defining target-specific
+// macros.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_BASIC_TARGETDEFINES_H
+#define LLVM_CLANG_LIB_BASIC_TARGETDEFINES_H
+
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/MacroBuilder.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace targets {
+/// DefineStd - Define a macro name and standard variants.  For example if
+/// MacroName is "unix", then this will define "__unix", "__unix__", and "unix"
+/// when in GNU mode.
+LLVM_LIBRARY_VISIBILITY
+void DefineStd(clang::MacroBuilder &Builder, llvm::StringRef MacroName,
+               const clang::LangOptions &Opts);
+
+LLVM_LIBRARY_VISIBILITY
+void defineCPUMacros(clang::MacroBuilder &Builder, llvm::StringRef CPUName,
+                     bool Tuning = true);
+
+LLVM_LIBRARY_VISIBILITY
+void addCygMingDefines(const clang::LangOptions &Opts,
+                       clang::MacroBuilder &Builder);
+} // namespace targets
+} // namespace clang
+#endif // LLVM_CLANG_LIB_BASIC_TARGETDEFINES_H
diff --git a/clang/lib/Basic/Targets.h b/clang/lib/Basic/Targets.h
index b4d2486b5d2b13..7b40b4388f8e23 100644
--- a/clang/lib/Basic/Targets.h
+++ b/clang/lib/Basic/Targets.h
@@ -15,32 +15,7 @@
 #ifndef LLVM_CLANG_LIB_BASIC_TARGETS_H
 #define LLVM_CLANG_LIB_BASIC_TARGETS_H
 
-#include "clang/Basic/LangOptions.h"
-#include "clang/Basic/MacroBuilder.h"
 #include "clang/Basic/TargetInfo.h"
-#include "llvm/ADT/StringRef.h"
+#include "TargetDefines.h"
 
-namespace clang {
-namespace targets {
-
-LLVM_LIBRARY_VISIBILITY
-std::unique_ptr<clang::TargetInfo>
-AllocateTarget(const llvm::Triple &Triple, const clang::TargetOptions &Opts);
-
-/// DefineStd - Define a macro name and standard variants.  For example if
-/// MacroName is "unix", then this will define "__unix", "__unix__", and "unix"
-/// when in GNU mode.
-LLVM_LIBRARY_VISIBILITY
-void DefineStd(clang::MacroBuilder &Builder, llvm::StringRef MacroName,
-               const clang::LangOptions &Opts);
-
-LLVM_LIBRARY_VISIBILITY
-void defineCPUMacros(clang::MacroBuilder &Builder, llvm::StringRef CPUName,
-                     bool Tuning = true);
-
-LLVM_LIBRARY_VISIBILITY
-void addCygMingDefines(const clang::LangOptions &Opts,
-                       clang::MacroBuilder &Builder);
-} // namespace targets
-} // namespace clang
 #endif // LLVM_CLANG_LIB_BASIC_TARGETS_H

Copy link

github-actions bot commented Nov 13, 2024

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

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes total sense to me. You should probably wait for more stamps here, but LGTM after minor nit


namespace clang {
namespace targets {
/// DefineStd - Define a macro name and standard variants. For example if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are moving, perhaps remove the DefineStd - in the comments.

@seven-mile seven-mile force-pushed the llvm/public-allocate-target branch from 7b870cc to 6d76970 Compare November 15, 2024 05:49
Copy link
Member

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

This is a perfectly reasonable change if these APIs are needed by CIR.

@Sirraide Sirraide merged commit acc3266 into llvm:main Nov 20, 2024
8 checks passed
smeenai pushed a commit to llvm/clangir that referenced this pull request Nov 23, 2024
…der (#1157)

With [llvm-project#116090](llvm/llvm-project#116090)
merged, we can get rid of `#include "../../../../Basic/Targets.h"` now.
lanza pushed a commit to llvm/clangir that referenced this pull request Mar 18, 2025
…der (#1157)

With [llvm-project#116090](llvm/llvm-project#116090)
merged, we can get rid of `#include "../../../../Basic/Targets.h"` now.
lanza pushed a commit to lanza/llvm-project that referenced this pull request Aug 9, 2025
…der (llvm#1157)

With [llvm-project#116090](llvm#116090)
merged, we can get rid of `#include "../../../../Basic/Targets.h"` now.
lanza pushed a commit to lanza/llvm-project that referenced this pull request Aug 11, 2025
…der (llvm#1157)

With [llvm-project#116090](llvm#116090)
merged, we can get rid of `#include "../../../../Basic/Targets.h"` now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants