This is an archive of the discontinued LLVM Phabricator instance.

Add runtime support for __cpu_model (__builtin_cpu_supports)
ClosedPublic

Authored by asbirlea on Jul 8 2016, 4:51 PM.

Details

Summary
This aims to add support for __cpu_model and address Bug 25510. It uses
the code from lib/Support/Host.cpp for cpu detection, and creates
__cpu_model with that info.

Tested on OSX and built on Linux as well (though libgcc is the default).
The use of "asm" required -std=gnu99, hence the cmake change. Corrections
on better addressing this are welcome.

Previously reverted, up for review again to iron out outstanding issues.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea updated this revision to Diff 63356.Jul 8 2016, 4:51 PM
asbirlea retitled this revision from to Add runtime support for __cpu_model (__builtin_cpu_supports).
asbirlea updated this object.
echristo edited edge metadata.Jul 11 2016, 3:05 PM

Couple of small comments, otherwise looks much better.

Thanks!

-eric

lib/builtins/cpu_model.c
16–18 ↗(On Diff #63356)

Can probably use a smaller set here maybe (and a couple of times later)?

Union of x86, x86_64, and windows provide is probably good enough.

126–147 ↗(On Diff #63356)

The comment and everything else feels weird here. Can you elaborate more? Also, the ifdefs seem unnecessary.

asbirlea updated this revision to Diff 63858.Jul 13 2016, 2:11 PM
asbirlea edited edge metadata.

Address comments.

Couple more inline comments, otherwise it's looking pretty good to me.

Thanks!

-eric

lib/builtins/cpu_model.c
130 ↗(On Diff #63858)

The syntax isn't for sure. :)

MSDN doesn't conditionalize the __cpuid intrinsic so I don't think this routine is necessary for MSVC and the return 1 is fine. Also, return true/false?

202 ↗(On Diff #63858)

The return values here seem duplicated with the isCpuIDSupported routine?

263 ↗(On Diff #63858)

Go ahead and document the return value and the arguments here please.

asbirlea updated this revision to Diff 63877.Jul 13 2016, 4:14 PM

Address comments.

asbirlea marked an inline comment as done.Jul 13 2016, 4:17 PM
asbirlea added inline comments.
lib/builtins/cpu_model.c
203 ↗(On Diff #63877)

I believe this covers more than isCpuIDSupported. Not sure I should modify/simplify it.

264 ↗(On Diff #63877)

Added a general comment describing the purpose of the method. Did you mean something more specific like "returns XCR0 in rEAX and rEDX"?

asbirlea updated this revision to Diff 64022.Jul 14 2016, 11:56 AM

Additional cleanup.

echristo accepted this revision.Jul 14 2016, 12:45 PM
echristo edited edge metadata.

I'm not sure the asserts are necessary, but I know you like them. That said, couple of inline comments then OK.

Thanks!

-eric

lib/builtins/cpu_model.c
193 ↗(On Diff #64022)

or MSVC.

205 ↗(On Diff #64022)

Add a FIXME to find out if we should save this for clang as well please?

243 ↗(On Diff #64022)

or MSVC

This revision is now accepted and ready to land.Jul 14 2016, 12:45 PM
joerg added inline comments.Jul 14 2016, 12:51 PM
lib/builtins/cpu_model.c
167 ↗(On Diff #64022)

Typo

echristo added inline comments.Jul 14 2016, 12:52 PM
lib/builtins/cpu_model.c
167 ↗(On Diff #64022)

To clarify: "Preserve".

asbirlea updated this revision to Diff 64040.Jul 14 2016, 1:51 PM
asbirlea edited edge metadata.

Address comments.

This revision was automatically updated to reflect the committed changes.

The patch introduced this: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/24764/steps/build%20with%20ninja/logs/stdio

FAILED: /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/bin/clang -D_DEBUG -D_GNU_SOURCE -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/builtins -I/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/builtins -Iinclude -I/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/include -gmlt -fPIC -Wall -W -Wno-unused-parameter -Wwrite-strings -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wdelete-non-virtual-dtor -Werror -Werror=date-time -fcolor-diagnostics -ffunction-sections -fdata-sections -Wall -Werror -Wno-unused-parameter -O3 -UNDEBUG -m32 -std=gnu99 -MMD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-i386.dir/cpu_model.c.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-i386.dir/cpu_model.c.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-i386.dir/cpu_model.c.o -c /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/builtins/cpu_model.c
/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/builtins/cpu_model.c:174:3: error: extension used [-Werror,-Wlanguage-extension-token]

asm("movl\t%%ebx, %%esi\n\t"
^

/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/builtins/cpu_model.c:224:3: error: extension used [-Werror,-Wlanguage-extension-token]

asm("movl\t%%ebx, %%esi\n\t"