This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Replace HasT2ExtractPack with HasDSP
ClosedPublic

Authored by samparker on Feb 7 2017, 2:04 AM.

Details

Summary

Removed the HasT2ExtractPack feature and replaced its references with HasDSP. This then allows the Thumb2 extend instructions to be selected for ARMv8M +dsp. These instruction descriptions have also been refactored and more target tests have been added for their isel.

Diff Detail

Repository
rL LLVM

Event Timeline

samparker created this revision.Feb 7 2017, 2:04 AM
zzheng added a subscriber: zzheng.Feb 7 2017, 3:01 PM
zzheng added inline comments.
lib/Target/ARM/ARMSubtarget.h
171 ↗(On Diff #87378)

Any reason they needed to be commented out rather than removed?

samparker added inline comments.Feb 8 2017, 1:09 AM
lib/Target/ARM/ARMSubtarget.h
171 ↗(On Diff #87378)

Ah, no! Just something I missed. Thanks.

samparker updated this revision to Diff 87610.Feb 8 2017, 1:22 AM

Removed a few lines which I had just commented out before.

rovka added a subscriber: rovka.Feb 10 2017, 6:49 AM

This looks mechanically sound to me, and I'm always in favor of removing subtarget features if we don't really need them. That being said, I'd appreciate it if someone that knows more about these extensions gave the final ok.

lib/Target/ARM/ARMInstrThumb2.td
1145 ↗(On Diff #87610)

You mean Requires DSP, right?

test/CodeGen/Thumb2/thumb2-sxt-uxt.ll
39 ↗(On Diff #87610)

You should share the same check prefixes for the similar cases, e.g. CHECK-NO-DSP for cortex-m3 and armv8m, and CHECK-DSP for the others. Then you don't have to repeat yourself in the checks :)

Same for the other test files.

41 ↗(On Diff #87610)

This is too tight, I think it's better to use CHECK-NOT: sxtab (like you did in the other test files) since you don't want to see sxtab _at all_ (the registers don't matter).

test/CodeGen/Thumb2/thumb2-uxt_rot.ll
29 ↗(On Diff #87610)

Same as above, get rid of the registers here.

test/CodeGen/Thumb2/thumb2-uxtb.ll
12 ↗(On Diff #87610)

Same as above (in the whole file)

Hi Diana,

Thanks for the testing tips, I'll make the changes. For the flags I found that no target specified T2Extract without specifying DSP and I only found a single test that referenced it. I also crossed checked the manuals and couldn't see why the extract instructions had been predicated on that specify flag.

Thanks,
sam

samparker updated this revision to Diff 88170.Feb 13 2017, 2:17 AM

Simplified the tests.

olista01 requested changes to this revision.Feb 15 2017, 2:29 AM

I've had a look through the architecture manuals, and I think we do need two different features here.

For example:

  • The QADD instruction (currently predicated on FeatureDSP) was introduced by the DSP extension to ARMv5 (aka ARMv5TE)
  • The SXTAB instruction (currently predicated on FeatureT2XtPk) was introduced by ARMv6 (but is also optionally available as part of the DSP extension to ARMv7-M)

Therefore, I think we need to keep two separate features. However, we may be able to make FeatureT2XtPk depend on FeatureDSP, as I haven't been able to find any architecture where SXTAB is available without QADD.

This revision now requires changes to proceed.Feb 15 2017, 2:29 AM

Hi Oli,

As far as I can see, instructions such as QADD aren't available for Thumb prior to T2 so predicating these instructions on IsThumb2 and HasDSP should cover them. I think the same is true for SXTAB, the difference between v7M and v7E-M targets is the HasDSP feature and the tests hopefully proved that SXTAB is only selected in the correct cases. Since HasT2ExtractPack was never defined without HasDSP, I can't see how this can break the expected functionality.

cheers

This revision is now accepted and ready to land.Feb 17 2017, 7:06 AM
This revision was automatically updated to reflect the committed changes.