This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rovka on Jun 16 2016, 7:05 AM.

Details

Summary

This is a cleanup commit similar to r271555, but for ARM.

The end goal is to get rid of the isSwift / isCortexXY / isWhatever methods.

Since the ARM backend seems to have quite a lot of calls to these methods, I intend to submit 5-6 subtarget features at a time, instead of one big lump.

Diff Detail

Repository
rL LLVM

Event Timeline

rovka updated this revision to Diff 60965.Jun 16 2016, 7:05 AM
rovka retitled this revision from to [ARM] Do not test for CPUs, use SubtargetFeatures (Part 1). NFC.
rovka updated this object.
rovka added subscribers: llvm-commits, rengolin.
rengolin added inline comments.Jun 16 2016, 7:17 AM
lib/Target/ARM/ARMInstrInfo.td
327 ↗(On Diff #60965)

probably don't need the comments any more?

rovka updated this revision to Diff 60972.Jun 16 2016, 7:27 AM

Remove redundant comments.

MatzeB edited edge metadata.Jun 16 2016, 11:44 AM

Looks mostly good to me, but:

lib/Target/ARM/ARMSubtarget.h
221–237 ↗(On Diff #60972)

This needs to be initialized, you may want to do a similar commit as I did in r271057 and use member initializers throughout the Subtarget class.

compnerd added inline comments.Jun 16 2016, 5:41 PM
lib/Target/ARM/ARM.td
111 ↗(On Diff #60972)

I think that it may be easier to actually have this be inverted and have a "cheap predicates" feature which would normally be false.

127 ↗(On Diff #60972)

Why not spell out prefer? I don't think that the two extra characters are that bad here.

132 ↗(On Diff #60972)

Similar.

137 ↗(On Diff #60972)

FeatureNEONFPMoves perhaps?

lib/Target/ARM/ARMInstrInfo.td
330 ↗(On Diff #60972)

Wrap these two lines? They seem beyond 80 characters.

rovka updated this revision to Diff 61075.Jun 17 2016, 2:38 AM
rovka edited edge metadata.
rovka marked 4 inline comments as done.

Addressed review comments.

rovka added a comment.Jun 17 2016, 2:39 AM

Thanks for having a look.

lib/Target/ARM/ARM.td
111 ↗(On Diff #60972)

This takes its name from TargetInstrInfo::IsProfitableToUnpredicate, which is where it's used. I'm not sure it's worth the trouble of renaming that for all the targets.

132 ↗(On Diff #60972)

Did some spelling out, but I'd rather not spell it out in the feature name, as that would make it go over 80 chars in the features list (and I'd rather keep feature names short than realign the whole thing). There's a precedent for this in FeaturePref32BitThumb.

137 ↗(On Diff #60972)

There's already a FeatureNEONForFP, I followed that one (although I'm not against renaming this or both of them).

While we're at it: I find it a bit peculiar that FeatureNEONForFP is enabled only for Swift and Cyclone, and FeatureNEONForFPMoves is enabled only for Cortex-A9. I'm not sure if this is an oversight or if I just picked a really bad name for this feature. Any thoughts?

lib/Target/ARM/ARMSubtarget.h
221–237 ↗(On Diff #60972)

Oops... Looks like for ARM this is done in ARMSubtarget::initializeEnvironment, so I added it there.

MatzeB added inline comments.Jun 20 2016, 10:18 AM
lib/Target/ARM/ARMSubtarget.h
221–237 ↗(On Diff #61075)

I find member initializers a lot less error prone than having the initializers in a separate method. But we can fix that in another commit.

rengolin added a subscriber: echristo.
sbaranga added inline comments.Jun 21 2016, 5:05 AM
lib/Target/ARM/ARM.td
137 ↗(On Diff #61075)

They are both core-specific optimizations, so it makes sense that they are enabled selectively:

FeatureNEONForFPMoves enables changing the execution domain for mov-like instructions from VFP to NEON and is a core-specific optimization, so it makes sense to me that it's only enabled on Cortex-A9 (unless we have additional data points).

FeatureNEONforFP forces almost all floating point operations to be performed with NEON instructions (which is a problem if we care about how denormals are handled), so it makes sense to me that it's only enabled for swift/cyclone.

I think the distinction is that FeatureNEONForFP uses NEON for VFP movs only when it minimizes the domain crossing between NEON and VFP, while FeatureNEONforFP does this everywhere (for movs and everything else). I'm not sure how you could improve the naming though.

lib/Target/ARM/ARMInstrInfo.td
329 ↗(On Diff #61075)

Is there any reason not to test for useNEONForFPMovs instead?

rengolin added inline comments.Jun 21 2016, 5:32 AM
lib/Target/ARM/ARM.td
137 ↗(On Diff #61075)

So, just because one is a sub-set of the other, makes sense to have the same name + radical. But the extra behaviour (NEON/VFP barrier profitability) could be documented, at least in its comment.

About Swift's behaviour to ignore NEON subnormal problem, we have the same issue at the IR level (loop vectorizer), and I have "solved" the old way "ST->isTargetDarwin()", which is true at the platform level.

If you're still not happy with the naming, maybe, such a property could be called "FeatureIgnoreDenormalFP"?

sbaranga added inline comments.Jun 21 2016, 6:25 AM
lib/Target/ARM/ARM.td
137 ↗(On Diff #61075)

Above it should have been "I think the distinction is that FeatureNEONForFPMoves uses NEON for VFP movs only when it minimizes the domain crossing between NEON and VFP, while FeatureNEONforFP does this everywhere (for movs and everything else). I'm not sure how you could improve the naming though.". Sorry if that was a little bit confusing.

I'm not too picky about the naming for either features, they could stay as they are (maybe with some added documentation). Just wanted to point out that the they way these heuristics are used is not an oversight and to give some context in case anyone could come up with a better name for these.

I'm happy with FeatureNEONforFP at the moment, I don't think there's any need to change it.

FWIW I think FeatureNEONforFP is not a superset of FeatureNEONForFPMoves. From a behavioural point of view, there's the heuristic difference. The implementations are also very different (the first being at ISel level, the second is done as part of post-RA optimization). But that's just my interpretation.

rengolin added inline comments.Jun 21 2016, 6:30 AM
lib/Target/ARM/ARM.td
137 ↗(On Diff #61075)

I'm happy with FeatureNEONforFP at the moment, I don't think there's any need to change it.

Ok, me too.

FWIW I think FeatureNEONforFP is not a superset of FeatureNEONForFPMoves. From a behavioural point of view, there's the heuristic difference. The implementations are also very different (the first being at ISel level, the second is done as part of post-RA optimization).

Well, it's about the meaning, not the behaviour. I think it's ok to have behaviour described on comments and meaning encoded in names.

rovka added inline comments.Jun 22 2016, 4:04 AM
lib/Target/ARM/ARM.td
137 ↗(On Diff #61075)

Ok, thanks. I'll try to add more comments.

lib/Target/ARM/ARMInstrInfo.td
329 ↗(On Diff #61075)

Not really, just that this seems to be used at ISel level, whereas useNEONForFPMovs is used for execution domain optimization, so I wasn't sure they were in the same boat. I can change it to useNEONForFPMovs if you think that makes sense.

lib/Target/ARM/ARMSubtarget.h
221–237 ↗(On Diff #61075)

Ok, I'll try to look into that after I'm done with the subtarget features.

MatzeB accepted this revision.Jun 22 2016, 10:50 AM
MatzeB edited edge metadata.

In any way LGTM from my side (but maybe wait for Renatos LGTM as well).

This revision is now accepted and ready to land.Jun 22 2016, 10:50 AM
rengolin accepted this revision.Jun 22 2016, 11:08 AM
rengolin added a reviewer: rengolin.

I'm happy with the patch, too. LGTM. Thanks!

lib/Target/ARM/ARMSubtarget.h
221–237 ↗(On Diff #61075)

Would be good to create a bugzilla about this, so we don't forget.

MatzeB added inline comments.Jun 22 2016, 11:16 AM
lib/Target/ARM/ARMSubtarget.h
221–237 ↗(On Diff #61075)

IMO switching to member initializers is obvious and trivial enough that it can just be done in an extra commit without review.

Thanks to everyone for the review.

lib/Target/ARM/ARMSubtarget.h
221–237 ↗(On Diff #61075)

Ok, I'm going to commit this now and then hopefully get the member initializers in later today.

This revision was automatically updated to reflect the committed changes.