[PATCH] D14183: [AArch64]Extend merging narrow loads into a wider load

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:192
@@ -173,1 +191,3 @@
+
+static bool isNarrowLd(unsigned Opc) {
   switch (Opc) {
----------------
Let's use "isNarrowLoad" here. No need for truncations when it adds nothing.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:594
@@ +593,3 @@
+    if (!IsUnscaled) {
+      assert((alignTo(OffsetImm, 2) == OffsetImm) &&
+             "Unexpected offset to merge");
----------------
Wouldn't it be more obvious to just use:

  assert(OffsetImm & 1 == 0)?

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:623
@@ -566,1 +622,3 @@
 
+    int immsHigh = getMemScale(I) == 1 ? 15 : 31;
+    int immrHigh = getMemScale(I) == 1 ? 8 : 16;
----------------
These magic numbers look very easy to get wrong. Is there no obvious programmatic way of deriving them?

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1227
@@ -1154,3 +1226,3 @@
   if (Paired != E) {
-    if (isSmallTypeLdMerge(MI)) {
-      ++NumSmallTypeMerged;
+    if (isNarrowLd(MI)) {
+      ++NumNarrowLdPromotion;
----------------
Why have you done this rename in the same patch? This clutters the patch.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1468
@@ -1391,3 +1467,3 @@
   // enabled only in cortex-a57 on which performance benefits were verified.
-  return ProfitableArch & (!SubTarget->requiresStrictAlign());
+  return ProfitableArch & (!Subtarget->requiresStrictAlign());
 }
----------------
This stuff should not be in the same patch as a functional change. Please separate out the patches.


http://reviews.llvm.org/D14183