This is an archive of the discontinued LLVM Phabricator instance.

[PATCH][X86] Slightly refactor default features for AMD bdver* cpus. Also improve checks for target features in test Preprocessor/predefined-arch-macros.c.
ClosedPublic

Authored by andreadb on Nov 5 2014, 9:20 AM.

Details

Summary

Hi Reid, Quentin (and all),

This patch slightly refactors how clang sets default features for AMD bdver cpus.
Method 'getDefaultFeatures' (in Basic/Targets.cpp), already uses a fallthrough mechanism to set default features for 'bdver4', bdver3 and bdver2. This patch simply adds another fallthrough from case 'CK_BDVER2' to case 'CK_BDVER1'. That is because 'bdver2' has the same features available in 'bdver1' plus BMI,FMA,F16C and TBM etc.

While at it, I added missing checks for features in test predefined-arch-macros.c.
In the case of BTVER2, this patch adds explicit checks for F16C, BMI and PCLMUL.
In the case of BDVER3 and BDVER4, this patch checks for the presence of FSGSBASE (added at r221130).

Please let me know if ok to submit.

Thanks,
Andrea

Diff Detail

Event Timeline

andreadb updated this revision to Diff 15815.Nov 5 2014, 9:20 AM
andreadb retitled this revision from to [PATCH][X86] Slightly refactor default features for AMD bdver* cpus. Also improve checks for target features in test Preprocessor/predefined-arch-macros.c..
andreadb updated this object.
andreadb edited the test plan for this revision. (Show Details)
andreadb added reviewers: rnk, craig.topper, qcolombet.
andreadb added a subscriber: Unknown Object (MLST).
craig.topper edited edge metadata.Nov 5 2014, 1:11 PM

Why have avx, fma4, and sse4a been added to CK_BDVER1? xop implies all of
those. Can we use comments instead?

Why have avx, fma4, and sse4a been added to CK_BDVER1? xop implies all of
those. Can we use comments instead?

Sure, I will remove the redundant calls to setFeatureEnabledImpl that set avx,fma4,sse4a.
I originally added those features mainly for clarity reason (to make more explicit that the target has avx). I agree that adding a comment is much better.
I will add a comment on top of the call that sets feature 'xop' for bdver1 that simply says "xop implies avx, fma4 and sse4a".

andreadb updated this revision to Diff 15828.Nov 5 2014, 2:15 PM
andreadb edited edge metadata.

Uploaded a new version of the patch that removes the redundant calls to 'setFeatureEnabledImpl' and adds a comment.

Please let me know what you think.

Thanks,
Andrea

Diffusion closed this revision.Nov 6 2014, 4:20 AM
Diffusion updated this revision to Diff 15852.

Closed by commit rL221449 (authored by adibiagio).

LGTM

Thanks Craig,
Committed revision 221449.

Cheers,
Andrea