[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

280

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

247

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

280

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

[igorb]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

247

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

[dsanders]

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

247

I've fixed this in the commit.

247

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

280

I've fixed this in the commit.

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

 /// It keeps internally the insertion point and debug location for all
 /// the new instructions we want to create.
 /// This information can be modify via the related setters.
 class MachineIRBuilder {
   /// MachineFunction under construction.
   MachineFunction *MF;
   /// Information used to access the description of the opcodes.
   const TargetInstrInfo *TII;
-  /// Information used to verify types are consistent.
-  const MachineRegisterInfo *MRI;
+  /// Information used to verify types are consistent and to create virtual registers.
+  MachineRegisterInfo *MRI;
   /// Debug location to be set to any instruction we create.
   DebugLoc DL;
 
   /// \name Fields describing the insertion point.
   /// @{
   MachineBasicBlock *MBB;
   MachineBasicBlock::iterator II;
   /// @}
public:
   /// \pre \p Res and \p Op0 must be generic virtual registers with pointer
   ///      type.
   /// \pre \p Op1 must be a generic virtual register with scalar type.
   ///
   /// \return a MachineInstrBuilder for the newly created instruction.
   MachineInstrBuilder buildGEP(unsigned Res, unsigned Op0,
                                unsigned Op1);
 
+  /// Materialize and insert \p Res<def> = G_GEP \p Op0, (G_CONSTANT \p Value)
+  ///
+  /// G_GEP adds \p Value bytes to the pointer specified by \p Op0,
+  /// storing the resulting pointer in \p Res. If \p Value is zero then no
+  /// G_GEP or G_CONSTANT will be created and \pre Op0 will be assigned to
+  /// \p Res.
+  ///
+  /// \pre setBasicBlock or setMI must have been called.
+  /// \pre \p Op0 must be a generic virtual register with pointer type.
+  /// \pre \p ValueTy must be a scalar type.
+  /// \pre \p Res must be 0. This is to detect confusion between
+  ///      materializeGEP() and buildGEP().
+  /// \post \p Res will either be a new generic virtual register of the same
+  ///       type as \p Op0 or \p Op0 itself.
+  ///
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  Optional<MachineInstrBuilder> materializeGEP(unsigned &Res, unsigned Op0,
+                                               const LLT &ValueTy,
+                                               uint64_t Value);
+
   /// Build and insert \p Res<def> = G_PTR_MASK \p Op0, \p NumBits
   ///
   /// G_PTR_MASK clears the low bits of a pointer operand without destroying its
   /// pointer properties. This has the effect of rounding the address *down* to
   /// a specified alignment in bits.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   /// \pre \p Res and \p Op0 must be generic virtual registers with pointer

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

case TargetOpcode::G_INSERT: {
     MIRBuilder.buildMerge(MI.getOperand(0).getReg(), DstRegs);
     MI.eraseFromParent();
     return Legalized;
   }
   case TargetOpcode::G_LOAD: {
     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> 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.materializeGEP(SrcReg, MI.getOperand(1).getReg(), LLT::scalar(64), Adjustment);
 
-      MIRBuilder.buildConstant(Offset, i * NarrowSize / 8);
-      MIRBuilder.buildGEP(SrcReg, MI.getOperand(1).getReg(), Offset);
       // 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: {

lib/CodeGen/GlobalISel/MachineIRBuilder.cpp

MachineInstrBuilder MachineIRBuilder::buildGEP(unsigned Res, unsigned Op0,
   assert(MRI->getType(Op1).isScalar()  && "invalid offset type");
 
   return buildInstr(TargetOpcode::G_GEP)
       .addDef(Res)
       .addUse(Op0)
       .addUse(Op1);
 }
 
+Optional<MachineInstrBuilder>
+MachineIRBuilder::materializeGEP(unsigned &Res, unsigned Op0,
+                                 const LLT &ValueTy, uint64_t Value) {
+  assert(Res == 0 && "Res is a result argument");
+  assert(ValueTy.isScalar()  && "invalid offset type");
+
+  if (Value == 0) {
+    Res = Op0;
+    return None;
+  }
+
+  Res = MRI->createGenericVirtualRegister(MRI->getType(Op0));
+  unsigned TmpReg = MRI->createGenericVirtualRegister(ValueTy);
+
+  buildConstant(TmpReg, Value);
+  return buildGEP(Res, Op0, TmpReg);
+}
+
 MachineInstrBuilder MachineIRBuilder::buildPtrMask(unsigned Res, unsigned Op0,
                                                    uint32_t NumBits) {
   assert(MRI->getType(Res).isPointer() &&
          MRI->getType(Res) == MRI->getType(Op0) && "type mismatch");
 
   return buildInstr(TargetOpcode::G_PTR_MASK)
       .addDef(Res)
       .addUse(Op0)

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)
 ...