Skip to content

Remove xbegin and _xend #126952

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 2 commits into from
Feb 22, 2025
Merged

Remove xbegin and _xend #126952

merged 2 commits into from
Feb 22, 2025

Conversation

DKLoehr
Copy link
Contributor

@DKLoehr DKLoehr commented Feb 12, 2025

intrin.h contains declarations for both xbegin and _xend, but they should already be included transitively from rtmintrin.h via immintrin.h and/or x86intrin.h. Having them in both places causes problems if both headers are included.

Furthermore, the intrin.h declaration of xbegin seems to be bugged anyway, since it's missing its leading underscore.

Fixes #95133

@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 Feb 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 12, 2025

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: Devon Loehr (DKLoehr)

Changes

intrin.h contains declarations for both xbegin and _xend, but they should already be included transitively from rtmintrin.h via immintrin.h and/or x86intrin.h. Having them in both places causes problems if both headers are included.

Furthermore, the intrin.h declaration of xbegin seems to be bugged anyway, since it's missing its leading underscore.

Fixes #95133


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

1 Files Affected:

  • (modified) clang/lib/Headers/intrin.h (-2)
diff --git a/clang/lib/Headers/intrin.h b/clang/lib/Headers/intrin.h
index 376046aeeaf5e..3dd1eb45817d4 100644
--- a/clang/lib/Headers/intrin.h
+++ b/clang/lib/Headers/intrin.h
@@ -162,8 +162,6 @@ void _Store_HLERelease(long volatile *, long);
 void _Store64_HLERelease(__int64 volatile *, __int64);
 void _StorePointer_HLERelease(void *volatile *, void *);
 void _WriteBarrier(void);
-unsigned __int32 xbegin(void);
-void _xend(void);
 
 /* These additional intrinsics are turned on in x64/amd64/x86_64 mode. */
 #if defined(__x86_64__) && !defined(__arm64ec__)

@rnk
Copy link
Collaborator

rnk commented Feb 12, 2025

This is an observable behavior change, so it should have a test. There's a test for this header at clang/test/Headers/ms-intrins.cpp that you can extend. You probably need to add a new RUN line that adds -mrtm or some other micro-architectural feature so that rtmintrin.h declares _xend and you observe the redeclaration error reported in #95133.

@RKSimon RKSimon requested review from rnk and RKSimon February 13, 2025 08:26
@DKLoehr
Copy link
Contributor Author

DKLoehr commented Feb 13, 2025

Added a test based on the linked bug. I couldn't get it to reproduce in anything that seemed to fit in the existing file, so I made a new one.

Copy link
Collaborator

@rnk rnk left a comment

Choose a reason for hiding this comment

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

Thanks!

@DKLoehr
Copy link
Contributor Author

DKLoehr commented Feb 21, 2025

Just checking in, this was approved a few days ago but hasn't been merged yet.

@phoebewang phoebewang merged commit bac4171 into llvm:main Feb 22, 2025
8 checks passed
@glandium
Copy link
Contributor

Should this be uplifted to release/20.x?
Cc: @tstellar

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.

_xend() declaration in intrin.h may cause function multiversioning error on Windows
5 participants