Skip to content

Commit

Permalink
Make LLVMRustHasFeature more robust
Browse files Browse the repository at this point in the history
The function should accept feature strings that old LLVM might not
support.

Simplify the code using the same approach used by
LLVMRustPrintTargetFeatures.

Dummify the function for non 4.0 LLVM and update the tests accordingly.
  • Loading branch information
lu-zero committed Jul 28, 2017
1 parent cbce0aa commit c471020
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 14 deletions.
20 changes: 7 additions & 13 deletions src/rustllvm/PassWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,20 +181,14 @@ extern "C" bool LLVMRustHasFeature(LLVMTargetMachineRef TM,
TargetMachine *Target = unwrap(TM);
const MCSubtargetInfo *MCInfo = Target->getMCSubtargetInfo();
const FeatureBitset &Bits = MCInfo->getFeatureBits();
const llvm::SubtargetFeatureKV *FeatureEntry;

#define SUBTARGET(x) \
if (MCInfo->isCPUStringValid(x##SubTypeKV[0].Key)) { \
FeatureEntry = x##FeatureKV; \
} else

GEN_SUBTARGETS { return false; }
#undef SUBTARGET

while (strcmp(Feature, FeatureEntry->Key) != 0)
FeatureEntry++;
#if LLVM_VERSION_GE(4, 0)
const ArrayRef<SubtargetFeatureKV> FeatTable = MCInfo->getFeatureTable();

return (Bits & FeatureEntry->Value) == FeatureEntry->Value;
for (auto &FeatureEntry : FeatTable)
if (!strcmp(FeatureEntry.Key, Feature))
return (Bits & FeatureEntry.Value) == FeatureEntry.Value;
#endif
return false;
}

enum class LLVMRustCodeModel {
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-make/print-cfg/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ all: default
$(RUSTC) --target x86_64-pc-windows-gnu --print cfg | grep x86_64
$(RUSTC) --target i686-pc-windows-msvc --print cfg | grep msvc
$(RUSTC) --target i686-apple-darwin --print cfg | grep macos
$(RUSTC) --target i686-unknown-linux-gnu --print cfg | grep sse2
$(RUSTC) --target i686-unknown-linux-gnu --print cfg | grep gnu

This comment has been minimized.

Copy link
@ranma42

ranma42 Aug 1, 2017

Contributor

I was unable to find the rationale behind this change. Did the changes to rustllvm regress this test?
This was intentionally designed to check for the correct detection of the SIMD functionality as explained in
#31709 (comment)

This comment has been minimized.

Copy link
@lu-zero

lu-zero Aug 1, 2017

Author Contributor

The test fails if sse2 is not available and it looked out of place considering the surrounding checks.

This comment has been minimized.

Copy link
@ranma42

ranma42 Aug 1, 2017

Contributor

The sse2 feature is supposed to be available on the i686-unknown-linux-gnu target (and AFAICT it should still have it).

I agree that it is not obvious that it is testing the target_feature field, but FWIW the previous lines are testing various elements in the configuration (specifically i686-pc-windows-msvc is testing target_env, x86_64-pc-windows-gnu is testing target_arch and target_os or target_family).

I agree that it is not obvious what each line is testing. Maybe instead of just gnu or msvc we should be grepping for target_env="msvc".


ifdef IS_WINDOWS
default:
Expand Down
1 change: 1 addition & 0 deletions src/test/run-pass/sse2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
// min-llvm-version 4.0

#![feature(cfg_target_feature)]

Expand Down

0 comments on commit c471020

Please sign in to comment.