This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add 'sahf' CPU feature to frontend
ClosedPublic

Authored by dim on Feb 16 2018, 9:06 AM.

Details

Summary

Make clang accept -msahf (and -mno-sahf) flags to activate the
+sahf feature for the backend, for bug 36028 (Incorrect use of
pushf/popf enables/disables interrupts on amd64 kernels). This was
originally submitted in bug 36037 by Jonathan Looney
<jonlooney@gmail.com>.

As described there, GCC also uses -msahf for this feature, and the
backend already recognizes the +sahf feature. All that is needed is to
teach clang to pass this on to the backend.

The mapping of feature support onto CPUs may not be complete; rather, it
was chosen to match LLVM's idea of which CPUs support this feature (see
lib/Target/X86/X86.td).

I also updated the affected test case (CodeGen/attr-target-x86.c) to
match the emitted output.

Diff Detail

Repository
rL LLVM

Event Timeline

dim created this revision.Feb 16 2018, 9:06 AM
craig.topper added inline comments.Feb 16 2018, 2:13 PM
lib/Basic/Targets/X86.cpp
295 ↗(On Diff #134642)

KNM and KNL should both have sahf

308 ↗(On Diff #134642)

sahf should be available on amdfam10

1049 ↗(On Diff #134642)

Does gcc define this? It's such a low level instruction I have a hard time believing this define would be useful.

emaste added a subscriber: emaste.Feb 16 2018, 2:17 PM
dim added inline comments.Feb 16 2018, 2:56 PM
lib/Basic/Targets/X86.cpp
1049 ↗(On Diff #134642)

I tried gcc 6, 7 and 8, and while they do expose stuff like __POPCNT__, I see no __LAHFSAHF__. I am supposing Jonathan's original intent with this was to make it easily testable in source, so you can insert the right assembly for the target CPU. The same could really be said for things like __RDSEED__, and some other defines...

craig.topper added inline comments.Feb 16 2018, 3:12 PM
lib/Basic/Targets/X86.cpp
1049 ↗(On Diff #134642)

Most of the defines indicate the availability of intrinsics. At least that was their original intent. They used to control what intrinsic header were included in x86intrin.h or immintrin.h. Though now we include everything except in MSVC compatible mode and allow the target attribute to provide per function control.

I'd prefer not to add this one if gcc doesn't have it.

dim updated this revision to Diff 134746.Feb 16 2018, 3:23 PM

Added the sahf feature for knm, knl and amdfam10 too.

dim marked 2 inline comments as done.Feb 16 2018, 3:28 PM
dim updated this revision to Diff 134808.Feb 17 2018, 10:45 AM

Remove the __LAHFSAHF__ predefined macro.

dim retitled this revision from [X86] Add 'sahf' CPU feature, and emit __LAHFSAHF__ for it to [X86] Add 'sahf' CPU feature to frontend.Feb 17 2018, 10:46 AM
dim edited the summary of this revision. (Show Details)
dim marked 3 inline comments as done.Feb 17 2018, 10:47 AM
This revision is now accepted and ready to land.Feb 17 2018, 12:40 PM
This revision was automatically updated to reflect the committed changes.