This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add XSAVE intrinsics (LLVM part)
ClosedPublic

Authored by aaboud on Sep 21 2015, 5:34 AM.

Details

Summary

Add intrinsics for the XSAVE instructions:
XSAVE, XSAVE64
XRSTOR, XRSTOR64
XSAVEOPT, XSAVEOPT64
XRSTORS, XRSTORS64
XSAVEC, XSAVEC64
XSAVES, XSAVES64

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 35233.Sep 21 2015, 5:34 AM
aaboud retitled this revision from to [X86] Add XSAVE intrinsics (LLVM part).
aaboud updated this object.
aaboud added reviewers: mkuper, delena, craig.topper.
aaboud set the repository for this revision to rL LLVM.
aaboud added a subscriber: llvm-commits.
mkuper added inline comments.Sep 27 2015, 12:04 AM
lib/Support/Host.cpp
827 ↗(On Diff #35233)

I think this is fine.

test/CodeGen/X86/system-intrinsics-64.ll
38 ↗(On Diff #35233)

Could you please add (in at least one of the tests) a check that the right things get copied into EAX and EDX?

craig.topper added inline comments.Sep 27 2015, 9:12 AM
lib/Target/X86/X86InstrSystem.td
487 ↗(On Diff #35233)

Aren't these all missing the feature predicate checks?

craig.topper added inline comments.Sep 27 2015, 9:18 AM
lib/Support/Host.cpp
830 ↗(On Diff #35233)

Is this needed? Bit 2 doesn't correspond to any instructions. It determines whether xgetbv allows ecx=1

craig.topper added inline comments.Sep 27 2015, 9:23 AM
lib/Support/Host.cpp
824 ↗(On Diff #35233)

Can we make this HasLeafD? I think the D would be more obvious capitalized.

aaboud updated this revision to Diff 35960.Sep 29 2015, 5:08 AM

Applied some changes according to Michael and Craig comments.

craig.topper added inline comments.Sep 29 2015, 10:52 AM
lib/Target/X86/X86.td
40 ↗(On Diff #35960)

These probably need to be added to their respective CPUs as well. But that can be done as a followup.

test/CodeGen/X86/system-intrinsics-64.ll
1 ↗(On Diff #35960)

The command line for this test doesn't enable xsaves or xsaveopt. Looks like its up to feature detection on the processor it runs on.

aaboud added a comment.Oct 7 2015, 1:48 AM

Thanks for the comment.
I will upload a new patch.

test/CodeGen/X86/system-intrinsics-64.ll
1 ↗(On Diff #35960)

I will fix that, see updated patch.

aaboud updated this revision to Diff 36715.Oct 7 2015, 1:55 AM

Fixed LIT tests by adding target specific attributes flag (-mattr).
To do that the LIT tests where split into separate files according to the attribute passed for each one.

craig.topper added inline comments.Oct 7 2015, 9:52 AM
lib/Target/X86/X86Subtarget.h
97 ↗(On Diff #36715)

I really wanted to approve this, but then I realized that these aren't set to false in the X86SubTarget constructor. Which I think is bad.

aaboud added inline comments.Oct 7 2015, 11:49 AM
lib/Target/X86/X86Subtarget.h
97 ↗(On Diff #36715)

No Problem :)
Will set them to false.
Is there anything else?

craig.topper accepted this revision.Oct 7 2015, 12:13 PM
craig.topper edited edge metadata.

Accepting assuming the X86Subtarget constructor will be fixed before commiting.

This revision is now accepted and ready to land.Oct 7 2015, 12:13 PM
This revision was automatically updated to reflect the committed changes.