[globalisel][legalizer] G_LOAD/G_STORE NarrowScalar should not emit G_GEP x, 0.

[dsanders]

Summary

When legalizing G_LOAD/G_STORE using NarrowScalar, we should avoid emitting
%0 = G_CONSTANT ty 0
%1 = G_GEP %x, %0
since it's cheaper to not emit the redundant instructions than it is to fold them
away later.

[qcolombet]

LGTM modulo a small refactoring to avoid code duplication.

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

288

Could you factorize the code in a helper function?

[dsanders]

Factored out the optimization into MachineIRBuilder.

@qcolombet: Before I commit, I'd just like to double-check that the new
materializeGEP() function in MachineIRBuilder is ok with you. The
materialize*() name is intended to be used for instructions that may have to
create vregs in some cases.

[qcolombet]

The new helper looks good.
Nitpicks below.

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

249

Is this clang-formated?
In the browser it looks larger than 80-col, but it may just be an impression.

288

Shouldn't we call materializeGEP here too for the whole if-then block?

[igorb]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

249

could you calculate offset type base on pointer size ? So it will be legal for 32bit targets .

[dsanders]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

249

I've fixed this in the commit.

249

Sure, I've made this change in the commit.

288

I've fixed this in the commit.

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

case TargetOpcode::G_LOAD: {
     int NumParts =
         MRI.getType(MI.getOperand(0).getReg()).getSizeInBits() / NarrowSize;
     LLT NarrowPtrTy = LLT::pointer(
         MRI.getType(MI.getOperand(1).getReg()).getAddressSpace(), NarrowSize);
 
     SmallVector<unsigned, 2> DstRegs;
     for (int i = 0; i < NumParts; ++i) {
       unsigned DstReg = MRI.createGenericVirtualRegister(NarrowTy);
-      unsigned SrcReg = MRI.createGenericVirtualRegister(NarrowPtrTy);
-      unsigned Offset = MRI.createGenericVirtualRegister(LLT::scalar(64));
+      unsigned SrcReg = 0;
+      unsigned Adjustment = i * NarrowSize / 8;
 
-      MIRBuilder.buildConstant(Offset, i * NarrowSize / 8);
+      if (Adjustment != 0) {
+        unsigned Offset = MRI.createGenericVirtualRegister(LLT::scalar(64));
+        SrcReg = MRI.createGenericVirtualRegister(NarrowPtrTy);
+        MIRBuilder.buildConstant(Offset, Adjustment);
         MIRBuilder.buildGEP(SrcReg, MI.getOperand(1).getReg(), Offset);
+      } else
+        SrcReg = MI.getOperand(1).getReg();
+
       // TODO: This is conservatively correct, but we probably want to split the
       // memory operands in the future.
       MIRBuilder.buildLoad(DstReg, SrcReg, **MI.memoperands_begin());
 
       DstRegs.push_back(DstReg);
     }
     unsigned DstReg = MI.getOperand(0).getReg();
     MIRBuilder.buildMerge(DstReg, DstRegs);
     MI.eraseFromParent();
     return Legalized;
   }
   case TargetOpcode::G_STORE: {
     unsigned NarrowSize = NarrowTy.getSizeInBits();
     int NumParts =
         MRI.getType(MI.getOperand(0).getReg()).getSizeInBits() / NarrowSize;
     LLT NarrowPtrTy = LLT::pointer(
         MRI.getType(MI.getOperand(1).getReg()).getAddressSpace(), NarrowSize);
 
     SmallVector<unsigned, 2> SrcRegs;
     extractParts(MI.getOperand(0).getReg(), NarrowTy, NumParts, SrcRegs);
 
     for (int i = 0; i < NumParts; ++i) {
-      unsigned DstReg = MRI.createGenericVirtualRegister(NarrowPtrTy);
+      unsigned DstReg = 0;
+      unsigned Adjustment = i * NarrowSize / 8;
+
+      if (Adjustment != 0) {
         unsigned Offset = MRI.createGenericVirtualRegister(LLT::scalar(64));
-      MIRBuilder.buildConstant(Offset, i * NarrowSize / 8);
+        DstReg = MRI.createGenericVirtualRegister(NarrowPtrTy);
+        MIRBuilder.buildConstant(Offset, Adjustment);
         MIRBuilder.buildGEP(DstReg, MI.getOperand(1).getReg(), Offset);
+      } else
+        DstReg = MI.getOperand(1).getReg();
+
       // TODO: This is conservatively correct, but we probably want to split the
       // memory operands in the future.
       MIRBuilder.buildStore(SrcRegs[i], DstReg, **MI.memoperands_begin());
     }
     MI.eraseFromParent();
     return Legalized;
   }
   case TargetOpcode::G_CONSTANT: {

test/CodeGen/AArch64/GlobalISel/legalize-load-store.mir

bb.0.entry:
     %5(s64) = G_LOAD %0 :: (load 8 from %ir.addr)
 
     ; CHECK: %6(p0) = G_LOAD %0(p0) :: (load 8 from %ir.addr)
     %6(p0) = G_LOAD %0(p0) :: (load 8 from %ir.addr)
 
     ; CHECK: %7(<2 x s32>) = G_LOAD %0(p0) :: (load 8 from %ir.addr)
     %7(<2 x s32>) = G_LOAD %0(p0) :: (load 8 from %ir.addr)
 
-    ; CHECK: [[OFFSET0:%[0-9]+]](s64) = G_CONSTANT i64 0
-    ; CHECK: [[GEP0:%[0-9]+]](p0) = G_GEP %0, [[OFFSET0]](s64)
-    ; CHECK: [[LOAD0:%[0-9]+]](s64) = G_LOAD [[GEP0]](p0) :: (load 16 from %ir.addr)
+    ; CHECK: [[LOAD0:%[0-9]+]](s64) = G_LOAD %0(p0) :: (load 16 from %ir.addr)
     ; CHECK: [[OFFSET1:%[0-9]+]](s64) = G_CONSTANT i64 8
     ; CHECK: [[GEP1:%[0-9]+]](p0) = G_GEP %0, [[OFFSET1]](s64)
     ; CHECK: [[LOAD1:%[0-9]+]](s64) = G_LOAD [[GEP1]](p0) :: (load 16 from %ir.addr)
     ; CHECK: %8(s128) = G_MERGE_VALUES [[LOAD0]](s64), [[LOAD1]](s64)
     %8(s128) = G_LOAD %0(p0) :: (load 16 from %ir.addr)
 ...
 
 ---
bb.0.entry:
 
     ; CHECK: G_STORE %5(s64), %0(p0) :: (store 8 into %ir.addr)
     %5(s64) = G_PTRTOINT %0(p0)
     G_STORE %5, %0 :: (store 8 into %ir.addr)
 
     ; CHECK: G_STORE %0(p0), %0(p0) :: (store 8 into %ir.addr)
     G_STORE %0(p0), %0(p0) :: (store 8 into %ir.addr)
 
-    ; CHECK: [[OFFSET0:%[0-9]+]](s64) = G_CONSTANT i64 0
-    ; CHECK: [[GEP0:%[0-9]+]](p0) = G_GEP %0, [[OFFSET0]](s64)
-    ; CHECK: G_STORE %5(s64), [[GEP0]](p0) :: (store 16 into %ir.addr)
+    ; CHECK: G_STORE %5(s64), %0(p0) :: (store 16 into %ir.addr)
     ; CHECK: [[OFFSET1:%[0-9]+]](s64) = G_CONSTANT i64 8
     ; CHECK: [[GEP1:%[0-9]+]](p0) = G_GEP %0, [[OFFSET1]](s64)
     ; CHECK: G_STORE %6(s64), [[GEP1]](p0) :: (store 16 into %ir.addr)
     %6(s64) = G_PTRTOINT %0(p0)
     %7(s128) = G_MERGE_VALUES %5, %6
     G_STORE %7, %0 :: (store 16 into %ir.addr)
 ...