...instead of redeclaring them in clang's own X86Target.def. They were already
required to be in sync (IIUC), so no reason to maintain two identical lists.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
| Time | Test | |
|---|---|---|
| 660 ms | x64 debian > libomp.lock::omp_init_lock.c |
Event Timeline
Comment Actions
Are these in the same order in X86TargetParser.Def? Can we make some comment in that file to make sure that we keep them in the order (see comment in original x86Target.def)?
Comment Actions
The order in X86Parser.def is determined by how bits are allocated in to feature vector in libgcc. They're not in any priority order that I know of.
Comment Actions
Another version. My main goal here is to gradually move more stuff from clang/
to llvm/ so I'm open to other suggestions in doing so.
| clang/lib/Basic/Targets/X86.cpp | ||
|---|---|---|
| 1073 | If all you care about is whether they are a consecutive range, why not just use std::is_sorted? | |
| clang/lib/Basic/Targets/X86.cpp | ||
|---|---|---|
| 1073 | The Priorities array isn't sorted. It's just whatever order the X86_FEATURE_COMPAT lists them. The values need to be unique and in a contiguous range. | |
| clang/lib/Basic/Targets/X86.cpp | ||
|---|---|---|
| 1073 | Then I'd suggest something like: llvm::sort, then assert *(end - 1) - *begin == std::distance(begin, end) && llvm::adjacent_find or something. I definitely didn't get that point out of this odd for-loop and is_contained. There is perhaps at trick with std::min and std::max too. Though, it looks like this is perhaps trying to prove that the range is 0 to the the array size, right? In that case, perhaps there is something easier. Also a nit, it is consecutive in that case. | |
| clang/lib/Basic/Targets/X86.cpp | ||
|---|---|---|
| 1073 | Actually... std::array<unsigned, array_lengthof(Priorities) -1> HelperList;
std::iota(HelperList.begin(), HelperList.end());
std::is_permutation(HelperList.begin(), HelperList.end(),
std::begin(Priorities), std::end(Priorities)); | |
| clang/lib/Basic/Targets/X86.cpp | ||
|---|---|---|
| 1073 | I thought about std::sort + std::iota + std::equal but wasn't sure how readable that would be with the extra helper object (range version isn't available in C++14) and hoped my version would be more compact. Since it wasn't obvious what it does I shamelessly copied your suggestion (and learnt about std::is_permutation and array_lengthof when doing it). Thanks! | |
Comment Actions
2 nits, give Craig a day or two to =1 as well please, particularly since he's the code-owner here.
| clang/lib/Basic/Targets/X86.cpp | ||
|---|---|---|
| 1063 | Just a nit, I would like a better description here of the constraint we're trying to enforce. I can imagine someone in the future failing this, and not getting the context without a few sentences of how no duplicates are allowed, and it must be all numbers from 0 to the-max-number. | |
| 1074 | ||
Just a nit, I would like a better description here of the constraint we're trying to enforce. I can imagine someone in the future failing this, and not getting the context without a few sentences of how no duplicates are allowed, and it must be all numbers from 0 to the-max-number.