This is an archive of the discontinued LLVM Phabricator instance.

[ARM] VFPv2 only supports 16 D registers.
ClosedPublic

Authored by efriedma on Sep 9 2019, 3:30 PM.

Details

Summary

r361845 changed the way we handle "D16" vs. "D32" targets; there used to be a negative "d16" which removed instructions from the instruction set, and now there's a "d32" feature which adds instructions to the instruction set. This is good, but there was an oversight in the implementation: the behavior of VFPv2 was changed. In particular, the "vfp2" feature was changed to imply "d32". This is wrong: VFPv2 only supports 16 D registers.

In practice, this means if you specify -mfpu=vfpv2, the compiler will generate illegal instructions.

This patch gets rid of "vfp2d16" and "vfp2d16sp", and fixes "vfp2" and "vfp2sp" so they don't imply "d32".

(There are a couple minor corresponding changes to clang; I'll post a corresponding patch soon.)

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Sep 9 2019, 3:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2019, 3:30 PM

This patch looks good, but presumably needs the clang half to be committed at the same time?

I considered suggesting using vfp2d16 and vfp2d16sp instead of vfp2 and vfp2sp to be consistent with the vfp3 feature names, but that would then be inconsistent with the user-facing name, so this way is fine.

efriedma updated this revision to Diff 219807.Sep 11 2019, 2:28 PM

Minor fix for ARMTargetParser: vfp2sp should be FPURestriction::SP_D16. I don't think this has any practical effect beyond making clang emit the "+vfp2sp" attribute in all the cases where it's legal.

Posted https://reviews.llvm.org/D67467 with the clang fix.

This revision is now accepted and ready to land.Sep 16 2019, 2:19 AM
This revision was automatically updated to reflect the committed changes.