Disallow generating vzeroupper before return instruction (iret) in interrupt handler function

[aaboud]

Summary

Resolved Bug 26412.
https://llvm.org/bugs/show_bug.cgi?id=26412

[nadav]

The patch LGTM.

Can you please move the comment above the line? I think it would make the code easier to follow.

[qcolombet]

Change the status to reflect Nadav’s LGTM.

[aaboud]

Minor change in a comment style, following Nadav's comment.

[DavidKreitzer]

Just one minor suggestion, Amjad. Otherwise, LGTM.

test/CodeGen/X86/x86-interrupt_vzeroupper.ll

7 ↗

(On Diff #48923)

This command line comment seems incorrect, since nehalem doesn't have feature AVX. Maybe you should change this to sandybridge (here and on lines 35-36)?

[ab]

lib/Target/X86/X86VZeroUpper.cpp

187 ↗

(On Diff #48923)

The name doesn't seem appropriate anymore, but I can't say I have a better idea.

test/CodeGen/X86/x86-interrupt_vzeroupper.ll

35–41 ↗

(On Diff #48923)

Can you simplify the test? The metadata and most attributes aren't necessary, and the remaining attributes (target-features) can be replaced with -mattr=+avx in the command line.

[aaboud]

Minor changes to follow Ahmed & David comments.

[DavidKreitzer]

LGTM, Amjad, thanks for the fixes.

[ab]

Sorry to be a nag, but the testcase can be simplified even further:

; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+avx < %s | FileCheck %s

; CHECK: vzeroupper
; CHECK-NEXT: call
; CHECK-NOT: vzeroupper
; CHECK: iret

define x86_intrcc void @foo(i8* %frame) {
  call void @bar()
  ret void
}

declare void @bar()

FWIW, I also don't think the C reproducer adds any value, but if you think otherwise feel free to keep it!

[aaboud]

In D17542#364850, @ab wrote:

Sorry to be a nag, but the testcase can be simplified even further:

; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+avx < %s | FileCheck %s

; CHECK: vzeroupper
; CHECK-NEXT: call
; CHECK-NOT: vzeroupper
; CHECK: iret

define x86_intrcc void @foo(i8* %frame) {
  call void @bar()
  ret void
}

declare void @bar()

FWIW, I also don't think the C reproducer adds any value, but if you think otherwise feel free to keep it!

OK, I modified the testcase and committed the patch.
Thanks for the review.

llvm/trunk/lib/Target/X86/X86VZeroUpper.cpp

struct BlockState {
       MachineBasicBlock::iterator FirstUnguardedCall;
     };
     typedef SmallVector<BlockState, 8> BlockStateMap;
     typedef SmallVector<MachineBasicBlock*, 8> DirtySuccessorsWorkList;
 
     BlockStateMap BlockStates;
     DirtySuccessorsWorkList DirtySuccessors;
     bool EverMadeChange;
+    bool IsX86INTR;
     const TargetInstrInfo *TII;
 
     static char ID;
   };
 
   char VZeroUpperInserter::ID = 0;
 }
 
void VZeroUpperInserter::processBasicBlock(MachineBasicBlock &MBB) {
 
   // Start by assuming that the block PASS_THROUGH, which implies no unguarded
   // calls.
   BlockExitState CurState = PASS_THROUGH;
   BlockStates[MBB.getNumber()].FirstUnguardedCall = MBB.end();
 
   for (MachineBasicBlock::iterator I = MBB.begin(); I != MBB.end(); ++I) {
     MachineInstr *MI = I;
-    bool isControlFlow = MI->isCall() || MI->isReturn();
+    // No need for vzeroupper before iret in interrupt handler function,
+    // epilogue will restore YMM registers if needed.
+    bool IsReturnFromX86INTR = IsX86INTR && MI->isReturn();
+    bool IsControlFlow = MI->isCall() || MI->isReturn();
 
     // Shortcut: don't need to check regular instructions in dirty state.
-    if (!isControlFlow && CurState == EXITS_DIRTY)
+    if ((!IsControlFlow || IsReturnFromX86INTR) && CurState == EXITS_DIRTY)
       continue;
 
     if (hasYmmReg(MI)) {
       // We found a ymm-using instruction; this could be an AVX instruction,
       // or it could be control flow.
       CurState = EXITS_DIRTY;
       continue;
     }
 
     // Check for control-flow out of the current function (which might
     // indirectly execute SSE instructions).
-    if (!isControlFlow)
+    if (!IsControlFlow || IsReturnFromX86INTR)
       continue;
 
     // If the call won't clobber any YMM register, skip it as well. It usually
     // happens on helper function calls (such as '_chkstk', '_ftol2') where
     // standard calling convention is not used (RegMask is not used to mark
     // register clobbered and register usage (def/imp-def/use) is well-defined
     // and explicitly specified.
     if (MI->isCall() && !callClobbersAnyYmmReg(MI))

 /// vzeroupper instructions before function calls.
 bool VZeroUpperInserter::runOnMachineFunction(MachineFunction &MF) {
   const X86Subtarget &ST = MF.getSubtarget<X86Subtarget>();
   if (!ST.hasAVX() || ST.hasAVX512() || ST.hasFastPartialYMMWrite())
     return false;
   TII = ST.getInstrInfo();
   MachineRegisterInfo &MRI = MF.getRegInfo();
   EverMadeChange = false;
+  IsX86INTR = MF.getFunction()->getCallingConv() == CallingConv::X86_INTR;
 
   bool FnHasLiveInYmm = checkFnHasLiveInYmm(MRI);
 
   // Fast check: if the function doesn't use any ymm registers, we don't need
   // to insert any VZEROUPPER instructions.  This is constant-time, so it is
   // cheap in the common case of no ymm use.
   bool YMMUsed = FnHasLiveInYmm;
   if (!YMMUsed) {

llvm/trunk/test/CodeGen/X86/x86-interrupt_vzeroupper.ll

+; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+avx < %s | FileCheck %s
+
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;; Checks that interrupt handler code does not call "vzeroupper" instruction
+;; before iret.
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+; CHECK: vzeroupper
+; CHECK-NEXT: call
+; CHECK-NOT: vzeroupper
+; CHECK: iret
+
+define x86_intrcc void @foo(i8* %frame) {
+  call void @bar()
+  ret void
+}
+
+declare void @bar()
+