This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Do not test for CPUs, use SubtargetFeatures (Part 3). NFCI
ClosedPublic

Authored by rovka on Jun 28 2016, 8:47 AM.

Details

Summary

This is a follow-up for r273544 and r273853.

The end goal is to get rid of the isSwift / isCortexXY / isWhatever methods.
This commit also marks them as obsolete.

Diff Detail

Repository
rL LLVM

Event Timeline

rovka updated this revision to Diff 62096.Jun 28 2016, 8:47 AM
rovka retitled this revision from to [ARM] Do not test for CPUs, use SubtargetFeatures (Part 3). NFCI.
rovka updated this object.
rovka added a subscriber: llvm-commits.
MatzeB added inline comments.Jun 28 2016, 11:17 AM
lib/Target/ARM/ARM.td
149–151 ↗(On Diff #62096)

This explanation is odd, the unwind data format is an operating system detail not something CPU dependent.

lib/Target/ARM/ARMSubtarget.h
398–400 ↗(On Diff #62096)

Does this patch remove the last uses of those functions? If yes, then I would recommend removing these functions, otherwise it is just too easy to find them with grep or autocompletion while missing the deprecation comment.

Maybe go with the aarch64 solution of exposing a getProcFamily() function (that forces you to look around for the enum and write a little bit more which may be enough to get people reading the comment and thinking :)

rengolin added inline comments.Jun 28 2016, 12:07 PM
lib/Target/ARM/ARMSubtarget.h
398–400 ↗(On Diff #62096)

The enum solution would be odd, since we're still leaving a lot of old and oddly chosen usage for later. Means we'll change a lot of "isCPUFOO" for "CPU == Foo" without reason.

MatzeB added inline comments.Jun 28 2016, 12:52 PM
lib/Target/ARM/ARMSubtarget.h
398–400 ↗(On Diff #62096)

Sure, that's why I asked whether that was the last use of these functions. I would only remove them once we have no users of them left.

rengolin added inline comments.Jun 28 2016, 1:55 PM
lib/Target/ARM/ARMSubtarget.h
398–400 ↗(On Diff #62096)

Sorry, my brain separated the two phrases in independent logical units. :)

rovka added inline comments.Jun 29 2016, 1:13 AM
lib/Target/ARM/ARM.td
149–151 ↗(On Diff #62096)

Hm, I think you're right about this, it probably shouldn't be a subtarget feature. I'll update the patch to undo this part for now, and maybe in time the isSwift check will evolve into an isDarwin or something.

lib/Target/ARM/ARMSubtarget.h
398–400 ↗(On Diff #62096)

These aren't the last remaining uses of these functions. There are also some in review in D21797 and D21798, and even more that I've documented here: https://llvm.org/bugs/show_bug.cgi?id=27992
I've marked the functions as obsolete for now because I intend to put this on hold for a while, and I don't want to come back and find 20 new uses besides the ones in PR27992 :)

rovka updated this revision to Diff 62202.Jun 29 2016, 3:55 AM

Removed UseStride4VFPs feature.

rengolin accepted this revision.Jul 6 2016, 2:00 AM
rengolin edited edge metadata.

Issues addressed, LGTM. Thanks!

This revision is now accepted and ready to land.Jul 6 2016, 2:00 AM
This revision was automatically updated to reflect the committed changes.