[PATCH] D29472: PR31358: Add support for armv7ve triple in llvm.

[Richard Barton via Phabricator via llvm-commits]

richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.
This revision now requires changes to proceed.

Hi Manoj

Virtualization requires TrustZone so I have added a few comments where I think this is missing. Looks like there are existing bugs in this area which you have propagated into your patch for ARMv7-VE. Would be good to fix the whole lot at once.

Shouldn't new TargetParser options also get a clang test for the new commandline? I was also expecting a build attributes test added to test/CodeGen/ARM/build-attributes.ll testing the Virtualization and Div attributes are set correctly.



================
Comment at: include/llvm/Support/ARMTargetParser.def:86
+ARM_ARCH("armv7ve", AK_ARMV7VE, "7VE", "v7ve", ARMBuildAttrs::CPUArch::v7,
+          FK_NEON, (ARM::AEK_VIRT | ARM::AEK_HWDIVARM | ARM::AEK_HWDIV |
+	  ARM::AEK_DSP))
----------------
The Virtualization extension requires TrustZone, so I would expect to see AEK_SEC in this list, unless this is implied by AEK_VIRT. I guess from the Divide macros also given that it is not.


================
Comment at: lib/Target/ARM/ARM.td:450
+                                                       FeatureHWDivARM,
+                                                       FeatureVirtualization,
+                                                       FeatureAClass,
----------------
Same comment here about lack of TrustZone with Virtualization. I think this should reallybe part of the FeatureVirtualization definition higher in the file. This definition also includes the two divide Features, so they are redundant here and can be removed. The same comment could perhaps be made for a number of other targets in this file.


================
Comment at: lib/Target/ARM/ARMSubtarget.h:55
     ARMv5tej, ARMv6, ARMv6k, ARMv6kz, ARMv6t2, ARMv6m, ARMv6sm, ARMv7a, ARMv7r,
-    ARMv7m, ARMv7em, ARMv8a, ARMv81a, ARMv82a, ARMv8mMainline, ARMv8mBaseline,
-    ARMv8r
+    ARMv7m, ARMv7em, ARMv7ve, ARMv8a, ARMv81a, ARMv82a, ARMv8mMainline,
+    ARMv8mBaseline, ARMv8r
----------------
Being super pedantic, after ARMv7-A would be a more logical order. Super super pedantic there


Repository:
  rL LLVM

https://reviews.llvm.org/D29472