[PATCH] Prevent repetitive warnings for unrecognized processors and features (2nd attempt)

[Artyom Skrobov]

>> Attached is the new patch, that includes an extensive refactoring of
>> MCSubtargetInfo and SubtargetFeatures, to prevent the repeated lookups of
>> CPU entries.
>> For the feature lookup failures, this still uses a set of strings to mark
>> the features whose lookup failed; but this set now belongs to the
>> MCSubtargetInfo instance.
>>
>> OK to commit in this new form?
>>
>>
>> Later on, this will probably be accompanied with a front-end fix, as
>> suggested by Jim.
>
> Any reason not to just fix this in the frontend to begin with? (is it
> going to be substantially harder)

One reason has already been named by Renato: this would need fixing in the
backend whether or not the Clang fix makes the invalid CPUs less accessible
to Clang users.

The other reason is that indeed the Clang driver is substantially more hairy
than MCSubtargetInfo: it has a few manually-crafted layers of target-strings
parsing, and the invalid CPUs are normally caught at that level; but
Cortex-R4 specifically somehow slips through. Not that it's strictly
invalid, either: Cortex-R4 does exist, and it is supported by ARM.
A proper fix for the driver would be to replace the whole cascade of
StringSwitch'es with something tablegen-driven, and ideally based on the
same tablegen files that drive the target selection in the backend. This is
a long-known FIXME, but it's unlikely to be finished in the short-term.

So, the best we can have in Clang in the short-term is a new
spur-of-the-moment hack to deny Cortex-R4, on top of the existing system
that had been known troublesome enough to maintain.

> That's a fair bit of code in LLVM. Also I'm assuming some of that
> code moved around and some of it is new - I don't know which bits
> is which, if this is the right way forward it might be helpful to
> have the refactoring separated from the semantic changes for
> greater visibility into the changes.

That's right, most of the code added to MCSubtargetInfo.cpp is in fact
carried over from SubtargetFeature.cpp, with many modifications every here
and there to adapt the methods to their new host class.

To facilitate a code review, I'll try to annotate the patch:

* class MCSubtargetInfo adopts SetImpliedBits(), ClearImpliedBits(), and
Help() as new private methods. These used to be static functions in
SubtargetFeature.cpp, so they no longer need to accept Bits, CPUTable,
CPUTableSize, FeatTable, or FeatTableSize via the parameters, and instead
they use the data members of MCSubtargetInfo: FeatureBits, ProcDesc,
NumProcs, ProcFeatures, NumFeatures correspondingly

* SubtargetFeatures::ToggleFeature() and SubtargetFeatures::getFeatureBits()
are inlined into their single calling points --
MCSubtargetInfo::ToggleFeature() and MCSubtargetInfo::InitMCProcessorInfo(),
correspondingly; same as above, they now use MCSubtargetInfo data members
instead of the parameters they used to accept

* for transplanting the body of SubtargetFeatures::ToggleFeature() into
MCSubtargetInfo, class SubtargetFeatures needs to export size() and
operator[], delegated to the underlying Features vector

* the static functions StripFlag(), isEnabled(), getLongestEntryLength() are
moved from SubtargetFeature.cpp to MCSubtargetInfo.cpp with no change

* the static function Find() is converted to accept a template-typed array
parameter, same as in the first version of the patch; otherwise, this
function is unchanged, although it is now moved from SubtargetFeature.cpp to
MCSubtargetInfo.cpp

* the function hasFlag(), which used to be static inline in
SubtargetFeature.cpp, is moved to SubtargetFeature.h and exported as an
inline function in namespace llvm. Otherwise, this function is unchanged

* a new static function reportUnrecognizedKey() in MCSubtargetInfo.cpp is
now the single point for reporting the failed lookups, which had previously
been copypasted in four distinct places in the code. The semantic change
(same as in the first version of the patch) is reporting the failures via
SMDiagnostic, instead of streaming them directly into errs

* class MCSubtargetInfo includes new data members CPUString, ProcEntry,
ProcSchedEntry, to keep track of the last CPU lookup. MCSubtargetInfo now
has a non-trivial constructor to set CPUString to "invalid-cpu-name". All of
this is a semantic change

* MCSubtargetInfo::InitMCProcessorInfo() caches the ProcEntry and
ProcSchedEntry that are looked up, and only reports lookup failures (via
reportUnrecognizedKey()) if called with a different CPU than the one cached.
This is a semantic change

* MCSubtargetInfo::getSchedModelForCPU() uses the cached ProcSchedEntry if
available: this is a semantic change. Also it now uses the templated Find()
for the lookup if a cached entry is not available, and uses
reportUnrecognizedKey() to report a lookup failure

* class MCSubtargetInfo includes the set of unrecognizedFeatures as a new
data member, and a new method findFeature() to perform both the lookup of a
feature entry, and the error reporting (via reportUnrecognizedKey()) on the
first failure for a given feature name. This is a semantic change

Hope this helps.