This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Reverse PostRASched subtarget feature logic
ClosedPublic

Authored by samparker on Aug 23 2017, 2:30 AM.

Details

Summary

Replace the UsePostRAScheduler SubtargetFeature with DisablePostRAScheduler, which is then used by Swift and Cyclone. This patch maintains enabling PostRA scheduling for other Thumb2 capable cores and/or for functions which are being compiled in ARM mode.

Diff Detail

Event Timeline

samparker created this revision.Aug 23 2017, 2:30 AM
fhahn edited edge metadata.Aug 30 2017, 2:13 AM

Thanks Sam! I think this should be in line with Matthias's comments in a previous review, but I think it would be good if he could have another quick look.

lib/Target/ARM/ARM.td
879

Lots of unrelated whitespace changes. I think @javed.absar submitted a change recently to aligned all Processors model fields consistently.

lib/Target/ARM/ARMSubtarget.cpp
362

Convention seems to be using a "getter" functions to access properties.

364

Is PostRAScheduler still defined in some models? If so, I think we should remove those references, as they would be misleading IMO

samparker updated this revision to Diff 113242.Aug 30 2017, 5:18 AM

Thanks Florian. I've removed my white space changes and introduced a customary getter function. I've also removed the PostRAScheduler flag from the Cortex-R52 model.

cheers,
sam

javed.absar added inline comments.Aug 30 2017, 6:29 AM
lib/Target/ARM/ARMSubtarget.cpp
364

may be add an explicit comment on why we choose to do this

samparker added inline comments.Aug 30 2017, 7:16 AM
lib/Target/ARM/ARMSubtarget.cpp
364

'Because the gods of old said so.' Honestly, I'm really not sure of the reason and the logic seems odd to me. Unfortunately a lot of tests rely on this behaviour and its not something that I want to look into at the moment.

javed.absar added inline comments.Aug 30 2017, 7:26 AM
lib/Target/ARM/ARMSubtarget.cpp
364

I think it is because of IT block - we don't want PostRA to mess it up.

samparker added inline comments.Aug 30 2017, 7:31 AM
lib/Target/ARM/ARMSubtarget.cpp
364

Ah, that makes sense, thanks!

MatzeB accepted this revision.Aug 30 2017, 2:29 PM

Thanks for looking into this again. This is indeed better. Did you choose the "negative" DisablePostRAScheduler feature over a "positive" FeaturePostRAScheduler just so it's less lines needed in the .td file? The positive variant seems slightly user friendlier and in line with aarch64 to me, even though it means adding the flag to every processor variant except for swift and cyclone.

Either way this is an improvement and should go in, LGTM.

lib/Target/ARM/ARMSubtarget.cpp
364

I don't know if that is the original reason / motivation for it. But indeed the machine scheduler was not prepared/had bugs when dealing with instruction bundles last time I looked and so would probably fail on IT blocks.

This revision is now accepted and ready to land.Aug 30 2017, 2:29 PM

Hi Matthias,

The reason for the inverse logic is because the decision isn't just based upon the CPU, but also the current mode. For the old CPUs, the post-RA scheduler should be used when compiling a function in arm mode, but not when in thumb mode because they only support Thumb1. I wanted to predicate on the Thumb2 capability, but then I would of still needed something else for cyclone and swift.

Thanks for the review,
sam

This revision was automatically updated to reflect the committed changes.