[Peephole] rewrite INSERT_SUBREG to SUBREG_TO_REG if upper bits zero

[Allen]

Summary

Restrict the 32-bit form of an instruction of integer as too many test cases
will be clobber as the register number updated.

From %reg = INSERT_SUBREG %reg, %subreg, subidx
To   %reg:subidx =  SUBREG_TO_REG 0, %subreg, subidx

Try to prefix the redundant mov instruction at D132325 as the SUBREG_TO_REG should not generate code.

[foad]

Nack. Whatever you are trying to do, I think this is the wrong way to do it.

rewrite INSERT_SUBREG to SUBREG_TO_REG if upper bits zero

Where do you actually check that the upper bits are zero?

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

1829–1831 ↗

(On Diff #456647)

This might be true for some instructions on some targets, but it is not true for Machine IR in general.

1840–1841 ↗

(On Diff #456647)

I don't think it's acceptable to restrict this to specific types.

[Allen]

In D132939#3758406, @foad wrote:

Where do you actually check that the upper bits are zero?

hi @foad , thanks for you attention, maybe I have some misunderstand. I try this refer to AArch64MIPeepholeOpt::visitORR in file AArch64MIPeepholeOpt.cpp.

[efriedma]

This transform should be in target-specific code so we can ensure the safety. (Or it could use a target hook if it really needs to be in TwoAddressInstructionPass for some reason.)

[Allen]

move this transform in target-specific code to ensure the safety

[Allen]

In D132939#3759126, @efriedma wrote:

This transform should be in target-specific code so we can ensure the safety. (Or it could use a target hook if it really needs to be in TwoAddressInstructionPass for some reason.)

Thanks @efriedma very much for your comment. And I move this transform in the peephole (target-specific code).

[Allen]

ping. Any further concerns? Thanks.

[efriedma]

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

261

If I'm understanding correctly, we're assuming the first operand to INSERT_SUBREG is irrelevant because a COPY would destroy the upper part of the register anyway? Maybe makes sense to explicitly note that.

281

Checking "!SrcMI" twice.

282

TargetOpcode::COPY <= TargetOpcode::GENERIC_OP_END should be true.

284

What are you trying to accomplish with these isTypeLegalForClass checks? You're looking for a register class that only contains integer registers? Can you just check that the register class is a subclass of GPR64all?

[Allen]

Address the comment

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

261

Ok, I add your comment., thanks

281

Thanks, delete the duplicate checking "!SrcMI"

282

Thanks, apply your comment

284

I add the isTypeLegalForClass checks because this changes will cause many test cases to be updated, especially some vector type register class.

def FPR64 : RegisterClass<"AArch64", [f64, i64, v2f32, v1f64, v8i8, v4i16, v2i32,
                                      v1i64, v4f16, v4bf16],
                                     64, (sequence "D%u", 0, 31)>;

Now I updated with AArch64::GPR64allRegClass.hasSubClassEq(RC) as your comment to check integer registers, thanks!

[efriedma]

LGTM

[MaskRay]

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

295

SubregMI caused a -Wunused-variable in -DLLVM_ENABLE_ASSERTIONS=off builds. I fixed it

[Allen]

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

295

oh, yes, thanks vey much, I see your commit f9b59249757

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

 //
 //    %3:gpr32 = ORRWrs $wzr, %2, 0
 //    %4:gpr64 = SUBREG_TO_REG 0, %3, %subreg.sub_32
 //
 //    If AArch64's 32-bit form of instruction defines the source operand of
 //    ORRWrs, we can remove the ORRWrs because the upper 32 bits of the source
 //    operand are set to zero.
 //
+// 5. %reg = INSERT_SUBREG %reg(tied-def 0), %subreg, subidx
+//     ==> %reg:subidx =  SUBREG_TO_REG 0, %subreg, subidx
+//
 //===----------------------------------------------------------------------===//
 
 #include "AArch64ExpandImm.h"
 #include "AArch64InstrInfo.h"
 #include "MCTargetDesc/AArch64AddressingModes.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/CodeGen/MachineDominators.h"
struct AArch64MIPeepholeOpt : public MachineFunctionPass {
   template <typename T>
   bool visitADDSUB(unsigned PosOpc, unsigned NegOpc, MachineInstr &MI);
   template <typename T>
   bool visitADDSSUBS(OpcodePair PosOpcs, OpcodePair NegOpcs, MachineInstr &MI);
 
   template <typename T>
   bool visitAND(unsigned Opc, MachineInstr &MI);
   bool visitORR(MachineInstr &MI);
+  bool visitINSERT(MachineInstr &MI);
   bool runOnMachineFunction(MachineFunction &MF) override;
 
   StringRef getPassName() const override {
     return "AArch64 MI Peephole Optimization pass";
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
bool AArch64MIPeepholeOpt::visitORR(MachineInstr &MI) {
   MRI->replaceRegWith(DefReg, SrcReg);
   MRI->clearKillFlags(SrcReg);
   LLVM_DEBUG(dbgs() << "Removed: " << MI << "\n");
   MI.eraseFromParent();
 
   return true;
 }
 
+bool AArch64MIPeepholeOpt::visitINSERT(MachineInstr &MI) {
+  // Check this INSERT_SUBREG comes from below zero-extend pattern.
+  //
+  // From %reg = INSERT_SUBREG %reg(tied-def 0), %subreg, subidx
+  // To   %reg:subidx =  SUBREG_TO_REG 0, %subreg, subidx
+  //
+  // We're assuming the first operand to INSERT_SUBREG is irrelevant because a
+  // COPY would destroy the upper part of the register anyway
+  if (!MI.isRegTiedToDefOperand(1))
+    return false;
+
+  Register DstReg = MI.getOperand(0).getReg();
+  const TargetRegisterClass *RC = MRI->getRegClass(DstReg);
+  MachineInstr *SrcMI = MRI->getUniqueVRegDef(MI.getOperand(2).getReg());
+  if (!SrcMI)
+    return false;
+
+  // From https://developer.arm.com/documentation/dui0801/b/BABBGCAC
+  //
+  // When you use the 32-bit form of an instruction, the upper 32 bits of the
+  // source registers are ignored and the upper 32 bits of the destination
+  // register are set to zero.
+  //
+  // If AArch64's 32-bit form of instruction defines the source operand of
+  // zero-extend, we do not need the zero-extend. Let's check the MI's opcode is
+  // real AArch64 instruction and if it is not, do not process the opcode
+  // conservatively.
+  if ((SrcMI->getOpcode() <= TargetOpcode::GENERIC_OP_END) ||
+      !AArch64::GPR64allRegClass.hasSubClassEq(RC))
+    return false;
+
+  // Build a SUBREG_TO_REG instruction
+  MachineInstr *SubregMI =
+      BuildMI(*MI.getParent(), MI, MI.getDebugLoc(),
+              TII->get(TargetOpcode::SUBREG_TO_REG), DstReg)
+          .addImm(0)
+          .add(MI.getOperand(2))
+          .add(MI.getOperand(3));
+  LLVM_DEBUG(dbgs() << MI << "  replace by:\n: " << *SubregMI << "\n");
+  MI.eraseFromParent();
+
+  return true;
+}
+
 template <typename T>
 static bool splitAddSubImm(T Imm, unsigned RegSize, T &Imm0, T &Imm1) {
   // The immediate must be in the form of ((imm0 << 12) + imm1), in which both
   // imm0 and imm1 are non-zero 12-bit unsigned int.
   if ((Imm & 0xfff000) == 0 || (Imm & 0xfff) == 0 ||
       (Imm & ~static_cast<T>(0xffffff)) != 0)
     return false;
 
bool AArch64MIPeepholeOpt::runOnMachineFunction(MachineFunction &MF) {
 
   bool Changed = false;
 
   for (MachineBasicBlock &MBB : MF) {
     for (MachineInstr &MI : make_early_inc_range(MBB)) {
       switch (MI.getOpcode()) {
       default:
         break;
+      case AArch64::INSERT_SUBREG:
+        Changed = visitINSERT(MI);
+        break;
       case AArch64::ANDWrr:
         Changed = visitAND<uint32_t>(AArch64::ANDWri, MI);
         break;
       case AArch64::ANDXrr:
         Changed = visitAND<uint64_t>(AArch64::ANDXri, MI);
         break;
       case AArch64::ORRWrs:
         Changed = visitORR(MI);

llvm/test/CodeGen/AArch64/peephole-insert-subreg.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -run-pass=aarch64-mi-peephole-opt -o - -mtriple=aarch64-unknown-linux -verify-machineinstrs %s | FileCheck %s
+
+--- |
+  define i64 @loop2(i32 noundef %width) {
+  entry:
+    %add = add i32 %width, -1
+    %zext = zext i32 %add to i64
+    %shl = shl nuw nsw i64 %zext, 1
+    ret i64 %shl
+  }
+
+...
+---
+---
+name:            loop2
+alignment:       4
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gpr32common, preferred-register: '' }
+  - { id: 1, class: gpr32common, preferred-register: '' }
+  - { id: 2, class: gpr64, preferred-register: '' }
+  - { id: 3, class: gpr64all, preferred-register: '' }
+  - { id: 4, class: gpr64, preferred-register: '' }
+liveins:
+  - { reg: '$w0', virtual-reg: '%0' }
+body: |
+  bb.0.entry:
+    liveins: $w0
+
+    ; CHECK-LABEL: name: loop2
+    ; CHECK: liveins: $w0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr32common = COPY $w0
+    ; CHECK-NEXT: [[SUBWri:%[0-9]+]]:gpr32common = SUBWri [[COPY]], 1, 0
+    ; CHECK-NEXT: [[DEF:%[0-9]+]]:gpr64all = IMPLICIT_DEF
+    ; CHECK-NEXT: [[SUBREG_TO_REG:%[0-9]+]]:gpr64 = SUBREG_TO_REG 0, killed [[SUBWri]], %subreg.sub_32
+    ; CHECK-NEXT: [[UBFMXri:%[0-9]+]]:gpr64 = nuw nsw UBFMXri killed [[SUBREG_TO_REG]], 63, 31
+    ; CHECK-NEXT: $x0 = COPY [[UBFMXri]]
+    ; CHECK-NEXT: RET_ReallyLR implicit $x0
+    %0:gpr32common = COPY $w0
+    %1:gpr32common = SUBWri %0, 1, 0
+    %3:gpr64all = IMPLICIT_DEF
+    %2:gpr64 = INSERT_SUBREG %3, killed %1, %subreg.sub_32
+    %4:gpr64 = nuw nsw UBFMXri killed %2, 63, 31
+    $x0 = COPY %4
+    RET_ReallyLR implicit $x0