Page MenuHomePhabricator

[ARM] Do not test for CPUs, use SubtargetFeatures. Also remove 1 flag
ClosedPublic

Authored by rovka on Jun 28 2016, 9:00 AM.

Details

Summary

This is a follow-up for r273544.

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

This commit also removes a command line flag that isn't used in any of the tests:
check-vmlx-hazards. It can be replaced easily with the mattr mechanism, since
this is now a subtarget feature.

I have mixed feelings about the FeatureExpandMLx. I'm not sure I understand the
history there. In particular, in the past MLx expansion was enabled for
subtargets with hasVFP2(), until r129775 [1] switched from that to isCortexA9,
without too much justification.

In spite of that, the code performing MLx expansion still contains calls to
isSwift/isLikeA9, although the results of those are pretty clear given that
we're only enabling it for the A9. Should we try to enable it for other targets?
If so, do we want to have a feature for it (as it is in this patch), or should
we control it based on other features? Is FeatureHasVMLxHazards a good candidate
for this? Any thoughts appreciated, thanks.

[1] http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20110418/119725.html

Diff Detail

Repository
rL LLVM

Event Timeline

rovka updated this revision to Diff 62101.Jun 28 2016, 9:00 AM
rovka retitled this revision from to [ARM] Do not test for CPUs, use SubtargetFeatures. Also remove 1 flag.
rovka updated this object.
rovka added a subscriber: llvm-commits.
rengolin edited edge metadata.Jun 29 2016, 2:15 AM

In particular, in the past MLx expansion was enabled for
subtargets with hasVFP2(), until r129775 [1] switched from that to isCortexA9,
without too much justification.

Now I remember this patch! This is about the VMLA+VMLA having a huge pipeline stall on A9, so VMLA+VMUL+VADD is preferred.

This is *exactly* the semantics of VMLxHazard and can be encoded in the way you do in this patch.

I remember this being a problem in A9, it may very well be present in A8 (though I don't remember exactly). I'm just surprised that A7 has the hazard as well. I'd have thought that, if A15 has fixed it, A7 would have carried the fix with it.

Regardless, this is a conversation for another patch. I think your fix is correct and it retains the current behaviour.

cheers,
--renato

rovka added a comment.Jul 6 2016, 12:49 AM

Ping? Can we commit this as-is?
If we want to switch to using HasVMLxHazards instead of ExpandMLx, I'm going to need some help with testing (at least on Swift), and it won't be NFC anymore (at least not on Swift, A7 and A8). I can bring it up on llvm-dev if there's interest.

MatzeB accepted this revision.Jul 6 2016, 11:13 AM
MatzeB edited edge metadata.

LGTM

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