[X86] Resolve FIXME: Create cld only when needed

[AZero13]

Only use cld when we also have rep instructions, are calling a function, or contain inline asm.

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: AtariDreams (AtariDreams)

Changes

---

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

1 Files Affected:

• (modified) llvm/lib/Target/X86/X86FrameLowering.cpp (+40-4)

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);

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

[RKSimon]

Based off #60527 - should we be also detecting use of std for a case to add cld at function exit? Or can we just rely on the InlineAsm case for now as I think that's the only way that std can occur (in which we should probably explicitly mention std instructions there)?

[AZero13]

Based off #60527 - should we be also detecting use of std for a case to add cld at function exit? Or can we just rely on the InlineAsm case for now as I think that's the only way that std can occur (in which we should probably explicitly mention std instructions there)?

I think we should rely on inline asm for now. Though, I am not aware that the compiler cannot emit an std instruction, right?

[AZero13]

Based off #60527 - should we be also detecting use of std for a case to add cld at function exit? Or can we just rely on the InlineAsm case for now as I think that's the only way that std can occur (in which we should probably explicitly mention std instructions there)?

Done!

[RKSimon]

Update the patch description:
"Only use cld when we also have rep instructions or are calling a function."
--> "Only use cld when we also have rep instructions, are calling a function or contain inline asm."?

[AZero13]

Update the patch description: "Only use cld when we also have rep instructions or are calling a function." --> "Only use cld when we also have rep instructions, are calling a function or contain inline asm."?

Done! @RKSimon

[RKSimon]

LGTM

[AZero13]

Test failure is unrelated to patch.

[RKSimon]

@AtariDreams Do you have commit access?

[AZero13]

@AtariDreams Do you have commit access?

@RKSimon I do not.

[gooncreeper]

@AtariDreams I don't know if I'm missing something, but shouldn't we emit cld for any string instruction, not just rep?

[AZero13]

#86557 I was thinking the same thing.

[topperc]

@AtariDreams I don't know if I'm missing something, but shouldn't we emit cld for any string instruction, not just rep?

I don't think the X86 backend ever emits a string instruction without REP.