Page MenuHomePhabricator

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

Authored by aaboud on Feb 23 2016, 6:27 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 48814.Feb 23 2016, 6:27 AM
aaboud retitled this revision from to Disallow generating vzeroupper before return instruction (iret) in interrupt handler function.
aaboud updated this object.
aaboud added reviewers: DavidKreitzer, hjl.tools, nadav.
aaboud added a subscriber: llvm-commits.
nadav edited edge metadata.Feb 23 2016, 7:07 AM
nadav added a subscriber: nadav.

The patch LGTM.

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

qcolombet accepted this revision.Feb 23 2016, 9:31 AM
qcolombet added a reviewer: qcolombet.
qcolombet added a subscriber: qcolombet.

Change the status to reflect Nadav’s LGTM.

This revision is now accepted and ready to land.Feb 23 2016, 9:31 AM
aaboud updated this revision to Diff 48923.Feb 24 2016, 6:20 AM
aaboud edited edge metadata.

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

DavidKreitzer edited edge metadata.Feb 24 2016, 2:13 PM

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 added a subscriber: ab.Feb 25 2016, 2:26 PM
ab added inline comments.
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 updated this revision to Diff 49374.Feb 29 2016, 7:20 AM
aaboud edited edge metadata.

Minor changes to follow Ahmed & David comments.

LGTM, Amjad, thanks for the fixes.

ab added a comment.Feb 29 2016, 5:16 PM

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!

This revision was automatically updated to reflect the committed changes.
aaboud added a comment.Mar 1 2016, 3:37 AM
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.