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

[Jun Bum Lim via llvm-commits]

junbuml added a comment.

Thanks  James for your review. I will address your comments and update it soon.


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:192
@@ -173,1 +191,3 @@
+
+static bool isNarrowLd(unsigned Opc) {
   switch (Opc) {
----------------
jmolloy wrote:
> Let's use "isNarrowLoad" here. No need for truncations when it adds nothing.
As you point out below, I will keep the same function name in this patch and will post a separate patch only for renaming.

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

================
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;
----------------
jmolloy wrote:
> These magic numbers look very easy to get wrong. Is there no obvious programmatic way of deriving them?
I agree with your comment. Let me update this in more programmatic way!

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1227
@@ -1154,3 +1226,3 @@
   if (Paired != E) {
-    if (isSmallTypeLdMerge(MI)) {
-      ++NumSmallTypeMerged;
+    if (isNarrowLd(MI)) {
+      ++NumNarrowLdPromotion;
----------------
jmolloy wrote:
> Why have you done this rename in the same patch? This clutters the patch.
Let me keep the same function name in this patch. I will add a separate patch only for renaming.

================
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());
 }
----------------
jmolloy wrote:
> This stuff should not be in the same patch as a functional change. Please separate out the patches.
This was a minor refactoring as D14534 introduced Subtarget as a member variable. I understand this is not perfectly related with this change. I will do this in a separate refactoring patch.



http://reviews.llvm.org/D14183