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

[Jim Grosbach]

On Feb 4, 2014, at 4:21 AM, Artyom Skrobov <Artyom.Skrobov at arm.com> wrote:

>>> 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.
> 

Why? I haven’t seen any reason for doing anything in the backend other than avoiding doing the work in the front end.


> 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.

Then that’s what should be fixed. Doing all of this backend work is not the right solution.

> 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.

Sounds like something that should be discussed with folks on cfe-commits/cfe-dev.

-Jim

> 
>> 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.
> 
> 
>