[PATCH] D26248: [AArch64] Removed the narrow load merging code in the ld/st optimizer.

[Jun Bum Lim via llvm-commits]

junbuml added a comment.

LGTM. Just minor inline comments. 
Thanks Chad for handling this.



================
Comment at: lib/Target/AArch64/AArch64.td:64
 
-def FeatureMergeNarrowLd : SubtargetFeature<"merge-narrow-ld",
-                                            "MergeNarrowLoads", "true",
-                                            "Merge narrow load instructions">;
+def FeatureMergeNarrowSt : SubtargetFeature<"merge-narrow-st",
+                                            "MergeNarrowStores", "true",
----------------
FeatureMergeNarrowZeroSt should be the right name.


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:41
           "Number of load/store from unscaled generated");
 STATISTIC(NumNarrowLoadsPromoted, "Number of narrow loads promoted");
 STATISTIC(NumZeroStoresPromoted, "Number of narrow zero stores promoted");
----------------
NumNarrowLoadsPromoted should be removed


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:551
 static bool isPromotableZeroStoreInst(MachineInstr &MI) {
   return (isPromotableZeroStoreOpcode(MI)) &&
          getLdStRegOp(MI).getReg() == AArch64::WZR;
----------------
isPromotableZeroStoreOpcode(MachineInstr &MI)  is used only in here.
By using isPromotableZeroStoreOpcode(MI.getOpcode()),  we can remove : 

static bool isPromotableZeroStoreOpcode(MachineInstr &MI) {
  return isPromotableZeroStoreOpcode(MI.getOpcode());
}


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1498
 bool AArch64LoadStoreOpt::optimizeBlock(MachineBasicBlock &MBB,
-                                        bool enableNarrowLdOpt) {
+                                        bool EnableNarrowStOpt) {
   bool Modified = false;
----------------
EnableNarrowStOpt -> EnableNarrowZeroStOpt  could be better.


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1712
   bool Modified = false;
-  bool enableNarrowLdOpt =
-    Subtarget->mergeNarrowLoads() && !Subtarget->requiresStrictAlign();
+  bool enableNarrowStOpt =
+      Subtarget->mergeNarrowStores() && !Subtarget->requiresStrictAlign();
----------------
enableNarrowStOpt -> EnableNarrowZeroStOpt


================
Comment at: lib/Target/AArch64/AArch64Subtarget.h:74
   bool StrictAlign = false;
-  bool MergeNarrowLoads = false;
+  bool MergeNarrowStores = false;
   bool UseAA = false;
----------------
MergeNarrowStores -> MergeNarrowZeroStores


================
Comment at: lib/Target/AArch64/AArch64Subtarget.h:182
   bool hasRAS() const { return HasRAS; }
-  bool mergeNarrowLoads() const { return MergeNarrowLoads; }
+  bool mergeNarrowStores() const { return MergeNarrowStores; }
   bool balanceFPOps() const { return BalanceFPOps; }
----------------
mergeNarrowStores -> mergeNarrowZeroStores


https://reviews.llvm.org/D26248