Skip to content

Adjust modulemap to mark mm3dnow as textual header. #107155

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
Sep 13, 2024

Conversation

jyknight
Copy link
Member

@jyknight jyknight commented Sep 3, 2024

This avoids issuing the deprecation diagnostic when building the module.

Not building it into a module shouldn't cause any negative impacts, since it no longer has any declarations other than the header guard. It's also very rarely included by anything.

Addresses #96246 (comment)

This avoids issuing the deprecation diagnostic when building the module.

Not building it into a module shouldn't cause any negative impacts,
since it no longer has any declarations other than the header
guard. It's also very rarely included by anything.

Addresses llvm#96246 (comment)
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Sep 3, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 3, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-x86

Author: James Y Knight (jyknight)

Changes

This avoids issuing the deprecation diagnostic when building the module.

Not building it into a module shouldn't cause any negative impacts, since it no longer has any declarations other than the header guard. It's also very rarely included by anything.

Addresses #96246 (comment)


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

1 Files Affected:

  • (modified) clang/lib/Headers/module.modulemap (+2-4)
diff --git a/clang/lib/Headers/module.modulemap b/clang/lib/Headers/module.modulemap
index 9ffc249c8d1a23..dcaf09e8f2c558 100644
--- a/clang/lib/Headers/module.modulemap
+++ b/clang/lib/Headers/module.modulemap
@@ -66,6 +66,8 @@ module _Builtin_intrinsics [system] [extern_c] {
     textual header "__wmmintrin_aes.h"
     textual header "__wmmintrin_pclmul.h"
 
+    textual header "mm3dnow.h"
+
     explicit module mm_malloc {
       requires !freestanding
       header "mm_malloc.h"
@@ -122,10 +124,6 @@ module _Builtin_intrinsics [system] [extern_c] {
       header "popcntintrin.h"
     }
 
-    explicit module mm3dnow {
-      header "mm3dnow.h"
-    }
-
     explicit module aes_pclmul {
       header "wmmintrin.h"
       export aes

@ian-twilightcoder
Copy link
Contributor

Does anything rely on the header guard? It would be good if we could remove it since textual headers are not generally expected to declare anything, even macros.

@jyknight
Copy link
Member Author

jyknight commented Sep 4, 2024

The only impact is that any modular header which includes this file will end up with a definition of the header guard, instead of importing it, right? It's not clear to me why it's useful to care about that?

(I totally do understand worrying about cases which result in e.g. having to merge 100s of duplicate function decls -- that can have noticeable performance impact.)

@ian-twilightcoder
Copy link
Contributor

The only impact is that any modular header which includes this file will end up with a definition of the header guard, instead of importing it, right? It's not clear to me why it's useful to care about that?

(I totally do understand worrying about cases which result in e.g. having to merge 100s of duplicate function decls -- that can have noticeable performance impact.)

Just good to not unintentionally add decls to pcms, and I think the intent of mm3dnow.h is not to add that define to the pcm, it's more like an implementation detail for include-once. But we don't really need to require include-once anymore with this one, and that'll save us writing unnecessary junk to the pcm of anything that includes this.

That said, there shouldn't be any significant impact either way.

@jyknight
Copy link
Member Author

jyknight commented Sep 4, 2024

If we don't keep the include-guard, then we'll report the same warning once per #include, instead of once per TU, which is probably not ideal.

@ian-twilightcoder
Copy link
Contributor

That's true. Probably nobody's using this header anymore anyway so either way should be fine I think.

@jyknight
Copy link
Member Author

I'll commit as soon as anyone gives it an approval. :)

@jyknight jyknight merged commit f902339 into llvm:main Sep 13, 2024
12 checks passed
@jyknight jyknight deleted the mm3dnow-textual branch September 13, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants