[AArch64]Enable the narrow ld promotion only on profitable microarchitectures

[junbuml]

Summary

The benefit from converting narrow loads into a wider loads (r251438) could be
micro-architecturally dependent, as it assumes that a single load with two bitfield
extracts is cheaper than two narrow loads. Currently, this conversion is enabled
only in cortex-a57 on which performance benefits were verified.

[mcrosier]

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1196

Why not call enableNarrowLdMerge(MBB->getParent()) here? You would need to change the logic in enableNarrowLdMerge since you're passing in a pointer to the MF, but that should be trivial.

1379

How about enableNarrowLdMerge, rather than couldNarrowLdMergeEnabled?

1395

I'd still prefer we sink this check into the optimizeBlock() function.

[junbuml]

Thanks Chad for the review. I addressed your comments.

[mcrosier]

LGTM. Thanks, Jun.

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1395

Jun and I discussed this offline. We shouldn't sink this because we don't need to call this predicate for every function. The answer will always be the same, so we only call it once in the runOnFunction function.

[junbuml]

committed in r252316.

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

typedef struct LdStPairFlags {
 
   void setSExtIdx(int V) { SExtIdx = V; }
   int getSExtIdx() const { return SExtIdx; }
 
 } LdStPairFlags;
 
 struct AArch64LoadStoreOpt : public MachineFunctionPass {
   static char ID;
-  AArch64LoadStoreOpt() : MachineFunctionPass(ID), IsStrictAlign(false) {
+  AArch64LoadStoreOpt() : MachineFunctionPass(ID) {
     initializeAArch64LoadStoreOptPass(*PassRegistry::getPassRegistry());
   }
 
   const AArch64InstrInfo *TII;
   const TargetRegisterInfo *TRI;
-  bool IsStrictAlign;
 
   // Scan the instructions looking for a load/store that can be combined
   // with the current instruction into a load/store pair.
   // Return the matching instruction if one is found, else MBB->end().
   MachineBasicBlock::iterator findMatchingInsn(MachineBasicBlock::iterator I,
                                                LdStPairFlags &Flags,
                                                unsigned Limit);
   // Merge the two instructions indicated into a single pair-wise instruction.
struct AArch64LoadStoreOpt : public MachineFunctionPass {
   // Merge a pre- or post-index base register update into a ld/st instruction.
   MachineBasicBlock::iterator
   mergeUpdateInsn(MachineBasicBlock::iterator I,
                   MachineBasicBlock::iterator Update, bool IsPreIdx);
 
   // Find and merge foldable ldr/str instructions.
   bool tryToMergeLdStInst(MachineBasicBlock::iterator &MBBI);
 
-  bool optimizeBlock(MachineBasicBlock &MBB);
+  // Check if converting two narrow loads into a single wider load with
+  // bitfield extracts could be enabled.
+  bool enableNarrowLdMerge(MachineFunction &Fn);
+
+  bool optimizeBlock(MachineBasicBlock &MBB, bool enableNarrowLdOpt);
 
   bool runOnMachineFunction(MachineFunction &Fn) override;
 
   const char *getPassName() const override {
     return AARCH64_LOAD_STORE_OPT_NAME;
   }
 };
 char AArch64LoadStoreOpt::ID = 0;
if (Paired != E) {
     // pain, so we let the merge routine tell us what the next instruction
     // is after it's done mucking about.
     MBBI = mergePairedInsns(MBBI, Paired, Flags);
     return true;
   }
   return false;
 }
 
-bool AArch64LoadStoreOpt::optimizeBlock(MachineBasicBlock &MBB) {
+bool AArch64LoadStoreOpt::optimizeBlock(MachineBasicBlock &MBB,
+                                        bool enableNarrowLdOpt) {
   bool Modified = false;
   // Three tranformations to do here:
   // 1) Find halfword loads that can be merged into a single 32-bit word load
   //    with bitfield extract instructions.
   //      e.g.,
   //        ldrh w0, [x2]
   //        ldrh w1, [x2, #2]
   //        ; becomes
bool AArch64LoadStoreOpt::optimizeBlock(MachineBasicBlock &MBB,
   //    as a base-reg writeback.
   //      e.g.,
   //        ldr x0, [x2]
   //        add x2, x2, #4
   //        ; becomes
   //        ldr x0, [x2], #4
 
   for (MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
-       !IsStrictAlign && MBBI != E;) {
+       enableNarrowLdOpt && MBBI != E;) {
     MachineInstr *MI = MBBI;
     switch (MI->getOpcode()) {
     default:
       // Just move on to the next instruction.
       ++MBBI;
       break;
     // Scaled instructions.
     case AArch64::LDRHHui:
for (MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
     }
       // FIXME: Do the other instructions.
     }
   }
 
   return Modified;
 }
 
+bool AArch64LoadStoreOpt::enableNarrowLdMerge(MachineFunction &Fn) {
+  const AArch64Subtarget *SubTarget =
+      &static_cast<const AArch64Subtarget &>(Fn.getSubtarget());
+  bool ProfitableArch = SubTarget->isCortexA57();
+  // FIXME: The benefit from converting narrow loads into a wider loads could be
+  // microarchitectural as it assumes that a single load with two bitfield
+  // extracts is cheaper than two narrow loads. Currently, this conversion is
+  // enabled only in cortex-a57 on which performance benefits were verified.
+  return ProfitableArch & (!SubTarget->requiresStrictAlign());
+}
+
 bool AArch64LoadStoreOpt::runOnMachineFunction(MachineFunction &Fn) {
   TII = static_cast<const AArch64InstrInfo *>(Fn.getSubtarget().getInstrInfo());
   TRI = Fn.getSubtarget().getRegisterInfo();
-  IsStrictAlign = (static_cast<const AArch64Subtarget &>(Fn.getSubtarget()))
-                      .requiresStrictAlign();
 
   bool Modified = false;
+  bool enableNarrowLdOpt = enableNarrowLdMerge(Fn);
   for (auto &MBB : Fn)
-    Modified |= optimizeBlock(MBB);
+    Modified |= optimizeBlock(MBB, enableNarrowLdOpt);
 
   return Modified;
 }
 
 // FIXME: Do we need/want a pre-alloc pass like ARM has to try to keep
 // loads and stores near one another?
 
 /// createAArch64LoadStoreOptimizationPass - returns an instance of the
 /// load / store optimization pass.
 FunctionPass *llvm::createAArch64LoadStoreOptimizationPass() {
   return new AArch64LoadStoreOpt();
 }

test/CodeGen/AArch64/arm64-ldp.ll

define i64 @ldp_sext_int_post(i32* %p) nounwind {
   %sexttmp = sext i32 %tmp to i64
   %sexttmp1 = sext i32 %tmp1 to i64
   %ptr = getelementptr inbounds i32, i32* %add.ptr, i64 1
   call void @use-ptr(i32* %ptr)
   %add = add nsw i64 %sexttmp1, %sexttmp
   ret i64 %add
 }
 
-; CHECK-LABEL: Ldrh_merge
-; CHECK-NOT: ldrh
-; CHECK: ldr [[NEW_DEST:w[0-9]+]]
-; CHECK: and w{{[0-9]+}}, [[NEW_DEST]], #0xffff
-; CHECK: lsr  w{{[0-9]+}}, [[NEW_DEST]]
-
-define i16 @Ldrh_merge(i16* nocapture readonly %p) {
-  %1 = load i16, i16* %p, align 2
-  ;%conv = zext i16 %0 to i32
-  %arrayidx2 = getelementptr inbounds i16, i16* %p, i64 1
-  %2 = load i16, i16* %arrayidx2, align 2
-  %add = add nuw nsw i16 %1, %2
-  ret i16 %add
-}
-
-; CHECK-LABEL: Ldurh_merge
-; CHECK-NOT: ldurh
-; CHECK: ldur [[NEW_DEST:w[0-9]+]]
-; CHECK: and w{{[0-9]+}}, [[NEW_DEST]], #0xffff
-; CHECK: lsr  w{{[0-9]+}}, [[NEW_DEST]]
-define i16 @Ldurh_merge(i16* nocapture readonly %p)  {
-entry:
-  %arrayidx = getelementptr inbounds i16, i16* %p, i64 -2
-  %0 = load i16, i16* %arrayidx
-  %arrayidx3 = getelementptr inbounds i16, i16* %p, i64 -1
-  %1 = load i16, i16* %arrayidx3
-  %add = add nuw nsw i16 %0, %1
-  ret i16 %add
-}
-
-; CHECK-LABEL: Ldrh_4_merge
-; CHECK-NOT: ldrh
-; CHECK: ldp [[NEW_DEST:w[0-9]+]]
-define i16 @Ldrh_4_merge(i16* nocapture readonly %P) {
-  %arrayidx = getelementptr inbounds i16, i16* %P, i64 0
-  %l0 = load i16, i16* %arrayidx
-  %arrayidx2 = getelementptr inbounds i16, i16* %P, i64 1
-  %l1 = load i16, i16* %arrayidx2
-  %arrayidx7 = getelementptr inbounds i16, i16* %P, i64 2
-  %l2 = load i16, i16* %arrayidx7
-  %arrayidx12 = getelementptr inbounds i16, i16* %P, i64 3
-  %l3 = load i16, i16* %arrayidx12
-  %add4 = add nuw nsw i16 %l1, %l0
-  %add9 = add nuw nsw i16 %add4, %l2
-  %add14 = add nuw nsw i16 %add9, %l3
-
-  ret i16 %add14
-}

test/CodeGen/AArch64/arm64-ldr-merge.ll

+; RUN: llc < %s -march=arm64 -mcpu=cortex-a57 -verify-machineinstrs | FileCheck %s
+
+; CHECK-LABEL: Ldrh_merge
+; CHECK-NOT: ldrh
+; CHECK: ldr [[NEW_DEST:w[0-9]+]]
+; CHECK: and w{{[0-9]+}}, [[NEW_DEST]], #0xffff
+; CHECK: lsr  w{{[0-9]+}}, [[NEW_DEST]]
+define i16 @Ldrh_merge(i16* nocapture readonly %p) {
+  %1 = load i16, i16* %p, align 2
+  %arrayidx2 = getelementptr inbounds i16, i16* %p, i64 1
+  %2 = load i16, i16* %arrayidx2, align 2
+  %add = add nuw nsw i16 %1, %2
+  ret i16 %add
+}
+
+; CHECK-LABEL: Ldurh_merge
+; CHECK-NOT: ldurh
+; CHECK: ldur [[NEW_DEST:w[0-9]+]]
+; CHECK: and w{{[0-9]+}}, [[NEW_DEST]], #0xffff
+; CHECK: lsr  w{{[0-9]+}}, [[NEW_DEST]]
+define i16 @Ldurh_merge(i16* nocapture readonly %p)  {
+entry:
+  %arrayidx = getelementptr inbounds i16, i16* %p, i64 -2
+  %0 = load i16, i16* %arrayidx
+  %arrayidx3 = getelementptr inbounds i16, i16* %p, i64 -1
+  %1 = load i16, i16* %arrayidx3
+  %add = add nuw nsw i16 %0, %1
+  ret i16 %add
+}
+
+; CHECK-LABEL: Ldrh_4_merge
+; CHECK-NOT: ldrh
+; CHECK: ldp [[NEW_DEST:w[0-9]+]]
+define i16 @Ldrh_4_merge(i16* nocapture readonly %P) {
+  %arrayidx = getelementptr inbounds i16, i16* %P, i64 0
+  %l0 = load i16, i16* %arrayidx
+  %arrayidx2 = getelementptr inbounds i16, i16* %P, i64 1
+  %l1 = load i16, i16* %arrayidx2
+  %arrayidx7 = getelementptr inbounds i16, i16* %P, i64 2
+  %l2 = load i16, i16* %arrayidx7
+  %arrayidx12 = getelementptr inbounds i16, i16* %P, i64 3
+  %l3 = load i16, i16* %arrayidx12
+  %add4 = add nuw nsw i16 %l1, %l0
+  %add9 = add nuw nsw i16 %add4, %l2
+  %add14 = add nuw nsw i16 %add9, %l3
+  ret i16 %add14
+}