[AArch64][SVE] Replace destructive operand of vector zeros with a bundled MOVPRFX instruction

[Allen]

Summary

Replace unary instructions where the destructive operand is a vector of zeros
with a bundled MOVPRFX instruction, e.g:

Transform:
    %X0 = DUP_ZI_S 0, 0
    %X0 = FLOGB_ZPmZ_S X0, P0, X2
 into:
     X0 = MOVPRFX P0/z, X1  // doesn't introduce any fake register dependencies compare to X0 = MOVPRFX P0/z, X0
     X0 = FLOGB_ZPmZ_S X0, P0, X2

NOTE: This patch add a @earlyclobber constraint to PredOneOpPassthruPseudo to ensure safe register allocation for movprfx usage.

Depends on D105889

[Allen]

hi @paulwalker-arm, would you please give me some suggestion, thanks

[paulwalker-arm]

In D138888#3959465, @Allen wrote:

hi @paulwalker-arm, would you please give me some suggestion, thanks

I'll investigate further but this looks like an old implementation that existed before the requirement to fix D105889. With D105889 in place we should be able to handle zeroing unary operations much like the existing experimental-zeroing support (i.e. within isel by creating _ZERO pseudo instructions that get expanded later), albeit the unary variants likely don't need to be experimental because we can better control their register allocation requirements.

[Allen]

refactor with comment

[Allen]

In D138888#3964294, @paulwalker-arm wrote:

In D138888#3959465, @Allen wrote:

hi @paulwalker-arm, would you please give me some suggestion, thanks

I'll investigate further but this looks like an old implementation that existed before the requirement to fix D105889. With D105889 in place we should be able to handle zeroing unary operations much like the existing experimental-zeroing support (i.e. within isel by creating _ZERO pseudo instructions that get expanded later), albeit the unary variants likely don't need to be experimental because we can better control their register allocation requirements.

hi, @paulwalker-arm

Thanks for your advice. As instriction flogb and fneg have some different form in DAG ISEL,  so I only try to enable flogb in this patch. （fneg will be transformed into FNEG_MERGE_PASSTHRU）

[Allen]

ping ?

[nikic]

(Not familiar with AArch64 / SVE)

[paulwalker-arm]

@Allen: I've been out of the office throughout January, hence the sparse reviewing. I'll be back to normal from tomorrow and will start working my way through the backlog.

[Allen]

In D138888#4096167, @paulwalker-arm wrote:

@Allen: I've been out of the office throughout January, hence the sparse reviewing. I'll be back to normal from tomorrow and will start working my way through the backlog.

Thank you advance

[paulwalker-arm]

Hi @Allen, again sorry for the delay. The patch looks mostly good but I think for the unary instructions the zeroing can be handled completely during isel with perhaps no changes to AArch64ExpandPseudoInsts.cpp necessary. Given the unary instructions have a dedicated operand for the inactive lanes I believe we can add a constraint to PredOneOpPassthruPseudo to ensure safe register allocation for movprfx usage. Something like:

let Constraints = !if(!eq(flags, FalseLanesZero), "$Zd = $Passthru,@earlyclobber $Zd", "");

This will ensure @passthru is allocated the same register as the destination whilst also being unique to the real input operand (i.e. the one containing the active lanes). The existing instruction expansion should emit:

movprfx	z0.h, p0/z, z1.h
flogb	z0.h, p0/m, z1.h

This is preferred over the output shown in sve2-intrinsics-fp-int-binary-logarithm-zeroing.ll because it doesn't introduce any fake register dependencies, which is something we've already fixed for the UNDEF variants.

What do you think?

llvm/lib/Target/AArch64/SVEInstrFormats.td

2798–2799 ↗

(On Diff #484509)

Are these changes required? sve_fp_2op_p_zd looks to already set DestructiveInstType accordingly.

llvm/test/CodeGen/AArch64/sve2-intrinsics-fp-int-binary-logarithm-zeroing.ll

3

I believe this is the default and thus not required?

[Allen]

Handle review suggestions based on the current modification solution.

I agree new solution that doesn't affect AArch64ExpandPseudoInsts.cpp seems better, I'm still trying.

[Allen]

llvm/lib/Target/AArch64/SVEInstrFormats.td

2798–2799 ↗

(On Diff #484509)

Thanks, When I try all the modifications of multiclass sve2_fp_flogb, an assert error occurs when I execute test case sve2-intrinsics-fp-int-binary-logarithm-zeroing.ll. Of course, the condition let DestructiveInstType = flags in is unnecessary, I'll delete it.

#2  0x0000aaaaaceef5c8 in llvm::AArch64InstPrinter::printInstruction (this=0xaaaab2be5f70, MI=0xffffffffc1c8, Address=0, STI=..., O=...)
    at lib/Target/AArch64/AArch64GenAsmWriter.inc:16963
16963	  assert(Bits != 0 && "Cannot print this instruction.");
(gdb) l
16958	  auto MnemonicInfo = getMnemonic(MI);
16959	
16960	  O << MnemonicInfo.first;
16961	
16962	  uint64_t Bits = MnemonicInfo.second;
16963	  assert(Bits != 0 && "Cannot print this instruction.");                 ----------- here -----------------------

llvm/test/CodeGen/AArch64/sve2-intrinsics-fp-int-binary-logarithm-zeroing.ll

3

Done, thanks

[Allen]

add @earlyclobber constraint according the comment, and revert the unnecessary changes to AArch64ExpandPseudoInsts.cpp

[paulwalker-arm]

A couple of stylistic requests but otherwise looks good.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td

3584–3586 ↗

(On Diff #494796)

We typically put these blocks just after the instructions they're pseudos for so can this block be placed just after defm FLOGB_ZPmZ?

llvm/lib/Target/AArch64/SVEInstrFormats.td

515 ↗

(On Diff #494796)

Can this be SVE_1_Op_PassthruZero_Pat? and moved further up so they follow the existing SVE_1_Op_PassthruUndef_.... classes.

[Allen]

sorry, missing the above review, and I'll fix that.

• Related link https://reviews.llvm.org/D143459

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

public:
   }
 
   bool runOnMachineFunction(MachineFunction &Fn) override;
 
   StringRef getPassName() const override { return AARCH64_EXPAND_PSEUDO_NAME; }
 
 private:
   bool expandMBB(MachineBasicBlock &MBB);
+  bool foldUnary(MachineInstr &MI, MachineBasicBlock &MBB,
+                 MachineBasicBlock::iterator MBBI);
   bool expandMI(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                 MachineBasicBlock::iterator &NextMBBI);
   bool expandMOVImm(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                     unsigned BitSize);
 
   bool expand_DestructiveOp(MachineInstr &MI, MachineBasicBlock &MBB,
                             MachineBasicBlock::iterator MBBI);
   bool expandCMP_SWAP(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
if (PRFX) {
     transferImpOps(MI, PRFX, DOP);
   } else
     transferImpOps(MI, DOP, DOP);
 
   MI.eraseFromParent();
   return true;
 }
 
+/// \brief Returns the defining instruction for MachineOperand MO,
+/// which should be a register. If we are searching for the defining
+/// instruction for the purpose of removing it, it only returns
+/// the defining instruction if it is not read between Def..*MBBI.
+static bool getRegisterDefInstr(MachineBasicBlock::iterator MBBI,
+                                const MachineOperand &MO,
+                                MachineBasicBlock::iterator &Def,
+                                bool ForRemoving = false) {
+  assert(MO.isReg() && "Operand must be a register");
+  unsigned Reg = MO.getReg();
+
+  if (MBBI == MBBI->getParent()->begin())
+    return false;
+
+  MachineBasicBlock::iterator RI = MBBI;
+  for (--RI; RI != MBBI->getParent()->begin(); --RI) {
+    // If we want to remove the Def, it cannot be *used* anywhere
+    // else in between the Def and MBBI
+    if (ForRemoving && !RI->definesRegister(Reg) && RI->readsRegister(Reg))
+      return false;
+    else if (RI->definesRegister(Reg))
+      break;
+  }
+
+  if (!RI->definesRegister(Reg))
+    return false;
+
+  Def = RI;
+  return true;
+}
+
+/// \brief Replace instructions where the destructive operand is
+/// a vector of zeros with a bundled MOVPRFX instruction, e.g.
+/// Transform:
+///    %X0 = DUP_ZI_S 0, 0
+///    %X0 = FNEG_ZPmZ_S X0, P0, X2
+/// into:
+///     X0 = MOVPRFX P0/z, X0
+///     X0 = FNEG_ZPmZ_S X0, P0, X2
+bool AArch64ExpandPseudo::foldUnary(MachineInstr &MI, MachineBasicBlock &MBB,
+                                    MachineBasicBlock::iterator MBBI) {
+  // Zd != Zn
+  if (MI.getOperand(0).getReg() == MI.getOperand(3).getReg())
+    return false;
+
+  // Zsd must be a DUP_ZI_(B|H|S|D) 0, 0
+  MachineBasicBlock::iterator Def;
+  if (!getRegisterDefInstr(MBBI, MI.getOperand(1), Def, true))
+    return false;
+
+  switch (Def->getOpcode()) {
+  case AArch64::DUP_ZI_B:
+  case AArch64::DUP_ZI_H:
+  case AArch64::DUP_ZI_S:
+  case AArch64::DUP_ZI_D:
+    break;
+  default:
+    return false;
+  }
+
+  if (!Def->getOperand(1).isImm() || Def->getOperand(1).getImm() != 0)
+    return false;
+
+  unsigned MovPrfx;
+  switch (TII->getElementSizeForOpcode(MI.getOpcode())) {
+  case AArch64::ElementSizeNone:
+  case AArch64::ElementSizeB:
+    MovPrfx = AArch64::MOVPRFX_ZPzZ_B;
+    break;
+  case AArch64::ElementSizeH:
+    MovPrfx = AArch64::MOVPRFX_ZPzZ_H;
+    break;
+  case AArch64::ElementSizeS:
+    MovPrfx = AArch64::MOVPRFX_ZPzZ_S;
+    break;
+  case AArch64::ElementSizeD:
+    MovPrfx = AArch64::MOVPRFX_ZPzZ_D;
+    break;
+  default:
+    llvm_unreachable("Unsupported ElementSize");
+  }
+
+  // Create a Zeroing MOVPRFX
+  MachineInstrBuilder PRFX, NewMI;
+  PRFX = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(MovPrfx))
+             .addReg(MI.getOperand(0).getReg(), RegState::Define)
+             .addReg(MI.getOperand(2).getReg())
+             .addReg(MI.getOperand(1).getReg(), RegState::Undef);
+
+  NewMI = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(MI.getOpcode()))
+              .add(MI.getOperand(0))
+              .addReg(MI.getOperand(1).getReg(), RegState::Kill)
+              .add(MI.getOperand(2))
+              .add(MI.getOperand(3));
+
+  finalizeBundle(MBB, PRFX->getIterator(), MBBI->getIterator());
+  transferImpOps(MI, PRFX, NewMI);
+
+  Def->eraseFromParent();
+  MBBI->eraseFromParent();
+
+  return true;
+}
+
 bool AArch64ExpandPseudo::expandSetTagLoop(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
     MachineBasicBlock::iterator &NextMBBI) {
   MachineInstr &MI = *MBBI;
   DebugLoc DL = MI.getDebugLoc();
   Register SizeReg = MI.getOperand(0).getReg();
   Register AddressReg = MI.getOperand(1).getReg();
 
bool AArch64ExpandPseudo::expandMI(MachineBasicBlock &MBB,
                                    MachineBasicBlock::iterator &NextMBBI) {
   MachineInstr &MI = *MBBI;
   unsigned Opcode = MI.getOpcode();
 
   // Check if we can expand the destructive op
   int OrigInstr = AArch64::getSVEPseudoMap(MI.getOpcode());
   if (OrigInstr != -1) {
     auto &Orig = TII->get(OrigInstr);
-    if ((Orig.TSFlags & AArch64::DestructiveInstTypeMask)
-           != AArch64::NotDestructive) {
+    if ((Orig.TSFlags & AArch64::DestructiveInstTypeMask) !=
+        AArch64::NotDestructive) {
       return expand_DestructiveOp(MI, MBB, MBBI);
     }
+  } else if ((MI.getDesc().TSFlags & AArch64::DestructiveInstTypeMask) ==
+             AArch64::DestructiveUnaryPassthru) {
+    return foldUnary(MI, MBB, MBBI);
   }
 
   switch (Opcode) {
   default:
     break;
 
   case AArch64::BSPv8i8:
   case AArch64::BSPv16i8: {

llvm/test/CodeGen/AArch64/sve2-intrinsics-fp-int-binary-logarithm-zeroing.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve2 -asm-verbose=1 < %s | FileCheck %s
+
+;
+; FLOGB
+;
+
+; NOTE: The %unused paramter ensures z0 is free, leading to a simpler test.
+define <vscale x 8 x i16> @flogb_f16(<vscale x 8 x i16> %unused, <vscale x 8 x i1> %pg, <vscale x 8 x half> %a) {
+; CHECK-LABEL: flogb_f16:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    movprfx z0.h, p0/z, z0.h
+; CHECK-NEXT:    flogb z0.h, p0/m, z1.h
+; CHECK-NEXT:    ret
+  %out = call <vscale x 8 x i16> @llvm.aarch64.sve.flogb.nxv8f16(<vscale x 8 x i16> zeroinitializer,
+                                                                 <vscale x 8 x i1> %pg,
+                                                                 <vscale x 8 x half> %a)
+  ret <vscale x 8 x i16> %out
+}
+
+define <vscale x 4 x i32> @flogb_f32(<vscale x 4 x i32> %unused, <vscale x 4 x i1> %pg, <vscale x 4 x float> %a) {
+; CHECK-LABEL: flogb_f32:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    movprfx z0.s, p0/z, z0.s
+; CHECK-NEXT:    flogb z0.s, p0/m, z1.s
+; CHECK-NEXT:    ret
+  %out = call <vscale x 4 x i32> @llvm.aarch64.sve.flogb.nxv4f32(<vscale x 4 x i32> zeroinitializer,
+                                                                 <vscale x 4 x i1> %pg,
+                                                                 <vscale x 4 x float> %a)
+  ret <vscale x 4 x i32> %out
+}
+
+define <vscale x 2 x i64> @flogb_f64(<vscale x 2 x i64> %unused, <vscale x 2 x i1> %pg, <vscale x 2 x double> %a) {
+; CHECK-LABEL: flogb_f64:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    movprfx z0.d, p0/z, z0.d
+; CHECK-NEXT:    flogb z0.d, p0/m, z1.d
+; CHECK-NEXT:    ret
+  %out = call <vscale x 2 x i64> @llvm.aarch64.sve.flogb.nxv2f64(<vscale x 2 x i64> zeroinitializer,
+                                                                 <vscale x 2 x i1> %pg,
+                                                                 <vscale x 2 x double> %a)
+  ret <vscale x 2 x i64> %out
+}
+
+;
+; FNEG
+;
+
+define <vscale x 4 x float> @fneg_f32(<vscale x 4 x float> %unused, <vscale x 4 x i1> %pg, <vscale x 4 x float> %a) {
+; CHECK-LABEL: fneg_f32:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    movprfx z0.s, p0/z, z0.s
+; CHECK-NEXT:    fneg z0.s, p0/m, z1.s
+; CHECK-NEXT:    ret
+  %out = call <vscale x 4 x float> @llvm.aarch64.sve.fneg.nxv4f32(<vscale x 4 x float> zeroinitializer,
+                                                                  <vscale x 4 x i1> %pg,
+                                                                  <vscale x 4 x float> %a)
+  ret <vscale x 4 x float> %out
+}
+
+declare <vscale x 8 x i16> @llvm.aarch64.sve.flogb.nxv8f16(<vscale x 8 x i16>, <vscale x 8 x i1>, <vscale x 8 x half>)
+declare <vscale x 4 x i32> @llvm.aarch64.sve.flogb.nxv4f32(<vscale x 4 x i32>, <vscale x 4 x i1>, <vscale x 4 x float>)
+declare <vscale x 2 x i64> @llvm.aarch64.sve.flogb.nxv2f64(<vscale x 2 x i64>, <vscale x 2 x i1>, <vscale x 2 x double>)
+declare <vscale x 4 x float> @llvm.aarch64.sve.fneg.nxv4f32(<vscale x 4 x float>, <vscale x 4 x i1>, <vscale x 4 x float>)