Page MenuHomePhabricator

[Targets] Move soft-float-abi filtering to `initFeatureMap`
ClosedPublic

Authored by george.burgess.iv on May 9 2019, 12:15 PM.

Details

Summary

I'm not convinced this is an excellent approach, in part because I'm unclear on where all we expect to funnel the results of TargetInfo::initFeatureMap. Thought I'd throw it out for review anyway, and see what people with actual context think. :)

The underlying problem I'm trying to solve is that, given the following code with a suitable ARM target,

___attribute__((target("crc"))) void foo() {}

clang ends up codegening a function with the '+soft-float-abi' target feature, which we go out of our way to remove in the frontend for the default target features, since the backend apparently tries to figure out whether the soft float ABI should be used on its own. This is more or less harmless on its own, but it causes the backend to emit a diagnostic directly to errs(). Rather than try to find a way to silence that diag, it seems better to not have to emit it in the first place.

An alternate fix would be to somehow try to filter this in cfe/lib/CodeGen, but there appear to be many callers of initFeatureMap; I get the vague feeling that we shouldn't be letting +soft-float-abi slip through any of them. Again, could be wrong about that due to my lack of familiarity with initFeatureMap's uses.

If we agree that this is a good way forward, there also appears to be +neonfp/-neonfp additions happening in handleTargetFeatures that should prooooobably be happening in initFeatureMap instead? If that's the case, I'm happy to do that as a follow-up, and make it so that handleTargetFeatures no longer mutates its input (which comments indicate would be desirable, along with a more general move of all of this initialization stuff to the construction of our various TargetInfo subclasses).

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2019, 12:15 PM

If we agree that this is a good way forward, there also appears to be +neonfp/-neonfp additions happening in handleTargetFeatures that should prooooobably be happening in initFeatureMap instead?

neonfp isn't passed as a feature in the first place; there's a separate API setFPMath which is used for that. We translate it into a target feature for the sake of the backend. So I'm not sure what you're proposing.


What happens if someone specifies attribute((target("soft-float-abi"))) or something like that? (There's an API isValidFeatureName to validate feature names, but we currently don't implement it.)

michaelplatings resigned from this revision.May 22 2019, 1:19 AM

Address comments -- thanks!

neonfp isn't passed as a feature in the first place; there's a separate API setFPMath which is used for that. We translate it into a target feature for the sake of the backend. So I'm not sure what you're proposing.

The idea would be for initFeatureMap to handle adding neonfp instead, so we can make the vector& we're passing into handleTargetFeatures a const vector&. An old comment claims that this would be desirable from a refactoring standpoint

What happens if someone specifies attribute((target("soft-float-abi"))) or something like that? (There's an API isValidFeatureName to validate feature names, but we currently don't implement it.)

Good catch. It doesn't appear that that works on its own for the target attribute, since we only filter user attrs, but it's probably something that's good to diagnose.

This revision is now accepted and ready to land.Jun 13 2019, 4:36 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 5:32 PM