-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[X86] Resolve FIXME: Create cld only when needed #82415
Conversation
@llvm/pr-subscribers-backend-x86 Author: AtariDreams (AtariDreams) ChangesFull diff: https://github.com/llvm/llvm-project/pull/82415.diff 1 Files Affected:
diff --git a/llvm/lib/Target/X86/X86FrameLowering.cpp b/llvm/lib/Target/X86/X86FrameLowering.cpp
index be416fb0db0695..569cd3330de2f2 100644
--- a/llvm/lib/Target/X86/X86FrameLowering.cpp
+++ b/llvm/lib/Target/X86/X86FrameLowering.cpp
@@ -2194,13 +2194,49 @@ void X86FrameLowering::emitPrologue(MachineFunction &MF,
// flag (DF in EFLAGS register). Clear this flag by creating "cld" instruction
// in each prologue of interrupt handler function.
//
- // FIXME: Create "cld" instruction only in these cases:
+ // Create "cld" instruction only in these cases:
// 1. The interrupt handling function uses any of the "rep" instructions.
// 2. Interrupt handling function calls another function.
//
- if (Fn.getCallingConv() == CallingConv::X86_INTR)
- BuildMI(MBB, MBBI, DL, TII.get(X86::CLD))
- .setMIFlag(MachineInstr::FrameSetup);
+ if (Fn.getCallingConv() == CallingConv::X86_INTR) {
+ bool NeedsCLD = false;
+
+ // Check if the function calls another function.
+ for (const BasicBlock &BB : Fn) {
+ for (const Instruction &I : BB) {
+ if (isa<CallInst>(I)) {
+ NeedsCLD = true;
+ break;
+ }
+
+ // Check for rep opcode.
+ if (auto *MI = dyn_cast<MachineInstr>(&I)) {
+ unsigned Opcode = MI->getOpcode();
+ if (Opcode == X86::REP_PREFIX || Opcode == X86::REPNE_PREFIX) {
+ NeedsCLD = true;
+ break;
+ }
+ }
+ }
+ }
+
+ // Check if the function uses any "rep" instructions.
+ if (!NeedsCLD) {
+ for (const BasicBlock &BB : Fn) {
+ for (const Instruction &I : BB) {
+ if (/* check if I is a "rep" instruction */) {
+ NeedsCLD = true;
+ break;
+ }
+ }
+ }
+ }
+
+ if (NeedsCLD) {
+ BuildMI(MBB, MBBI, DL, TII.get(X86::CLD))
+ .setMIFlag(MachineInstr::FrameSetup);
+ }
+ }
// At this point we know if the function has WinCFI or not.
MF.setHasWinCFI(HasWinCFI);
|
|
bdc74a8
to
921ac0d
Compare
7b17736
to
37ab03e
Compare
d1e2c47
to
91c0f9e
Compare
9b026ca
to
d2326d2
Compare
Based off #60527 - should we be also detecting use of |
I think we should rely on inline asm for now. Though, I am not aware that the compiler cannot emit an std instruction, right? |
Done! |
fe5cc17
to
af5a206
Compare
Update the patch description: |
Done! @RKSimon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Test failure is unrelated to patch. |
959987a
to
ce2b03c
Compare
Only use cld when we also have rep instructions, are calling a function, or contain inline asm.
@AtariDreams Do you have commit access? |
@RKSimon I do not. |
@AtariDreams I don't know if I'm missing something, but shouldn't we emit cld for any string instruction, not just rep? |
#86557 I was thinking the same thing. |
I don't think the X86 backend ever emits a string instruction without REP. |
Only use cld when we also have rep instructions, are calling a function, or contain inline asm.