[PATCH] D34142: [ARM] Add macro fusion for AES instructions.

[Javed Absar via Phabricator via llvm-commits]

javed.absar added a comment.

Thanks Florian for this. Some comments  below -



================
Comment at: lib/Target/ARM/ARM.td:103
                 "Enable fast computation of positive address offsets">;
-
+def FeatureFuseAES : SubtargetFeature<
+    "fuse-aes", "HasFuseAES", "true",
----------------
nitpick - Formatting seems different from rest of defs in this file.
 ...  : SubtargetFeature<"fuse-aes", "HasFuseAES", "true",


================
Comment at: lib/Target/ARM/ARMMacroFusion.cpp:41
+
+  if (ST.hasFuseAES())
+    // Fuse AES crypto operations.
----------------
Should this check be upfront so it comes out immediately? 

In fact, to TargetMachine.cpp, for efficiency ?
const ARMSubtarget &ST = C->MF->getSubtarget<ARMSubtarget>();
if (ST.hasFuseAES()) 
  DAG->addMutation(createARMMacroFusionDAGMutation());


================
Comment at: lib/Target/ARM/CMakeLists.txt:53
   ARMMachineFunctionInfo.cpp
+  ARMMacroFusion.cpp
   ARMRegisterInfo.cpp
----------------
why add twice?


https://reviews.llvm.org/D34142