[AArch64] Remove isDef32

[dmgreen]

Summary

isDef32 would attempt to make a guess at which SelectionDag nodes were 32bit sources, and use the nature of 32bit AArch64 instructions implicitly zeroing the upper register half to not emit zext that were expected to already be zero. This was a bit fragile though, needing to guess at the correct opcodes that do not become 32bit defs.

This patch removed isDef32, relying on the AArch64MIPeephole optimizer to remove redundant SUBREG_TO_REG nodes. A part of SelectArithExtendedRegister was left with the same logic as a heuristic to prevent some regressions from it picking less optimal sequences, and the AArch64MIPeepholeOpt needs to be taught that COPY from a FPR will become a FMOVSWr, doing the same implicit zeroing.

Fixes #55833

[efriedma]

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

827–828

Maybe explicitly note this is a heuristic, so it doesn't matter if it's wrong in edge cases.

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

209

It's not clear to me we'll always end up with an FMOVSWr, given copy coalescing.

You could explicitly select an FMOVSWr here, but I guess that has other consequences for register allocation...

218

Should we check for pseudo-instructions here? Are there any pseudo-instructions which might produce a register with undefined high bits?

[dmgreen]

Lower COPY to FMOVSWr and add a comment about the heuristic

[dmgreen]

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

209

We can try it and see how it goes. I'm hoping it is fairly rare that it both is used by a redundant ORR and has negative consequences for register allocation.

218

Do you mean the AArch64 pseudos? Do you have any instructions in mind?

This is a list of the pseudos, minus the SVE instructions and G_ pseudos:

ADDSWrr     = 249,
ADDSXrr     = 250,
ADDWrr      = 251,
ADDXrr      = 252,
ADDlowTLS   = 257,
ADJCALLSTACKDOWN    = 258,
ADJCALLSTACKUP      = 259,
AESIMCrrTied        = 260,
AESMCrrTied = 261,
ANDSWrr     = 262,
ANDSXrr     = 263,
ANDWrr      = 264,
ANDXrr      = 265,
BICSWrr     = 286,
BICSXrr     = 287,
BICWrr      = 288,
BICXrr      = 289,
BLRNoIP     = 294,
BLR_BTI     = 295,
BLR_RVMARKER        = 296,
BSPv16i8    = 297,
BSPv8i8     = 298,
CATCHRET    = 299,
CLEANUPRET  = 300,
CMP_SWAP_128        = 309,
CMP_SWAP_128_ACQUIRE        = 310,
CMP_SWAP_128_MONOTONIC      = 311,
CMP_SWAP_128_RELEASE        = 312,
CMP_SWAP_16 = 313,
CMP_SWAP_32 = 314,
CMP_SWAP_64 = 315,
CMP_SWAP_8  = 316,
CompilerBarrier     = 325,
EMITBKEY    = 326,
EONWrr      = 327,
EONXrr      = 328,
EORWrr      = 329,
EORXrr      = 330,
F128CSEL    = 335,
FMOVD0      = 440,
FMOVH0      = 441,
FMOVS0      = 442,
HOM_Epilog  = 671,
HOM_Prolog  = 672,
HWASAN_CHECK_MEMACCESS      = 673,
HWASAN_CHECK_MEMACCESS_SHORTGRANULES
IRGstack    = 675,
JumpTableDest16     = 676,
JumpTableDest32     = 677,
JumpTableDest8      = 678,
LOADgot     = 730,
MOPSMemoryCopyPseudo        = 755,
MOPSMemoryMovePseudo        = 756,
MOPSMemorySetPseudo = 757,
MOPSMemorySetTaggingPseudo  = 758,
MOVMCSym    = 759,
MOVaddr     = 760,
MOVaddrBA   = 761,
MOVaddrCP   = 762,
MOVaddrEXT  = 763,
MOVaddrJT   = 764,
MOVaddrTLS  = 765,
MOVbaseTLS  = 766,
MOVi32imm   = 767,
MOVi64imm   = 768,
ORNWrr      = 781,
ORNXrr      = 782,
ORRWrr      = 783,
ORNXrr      = 782,
ORRWrr      = 783,
ORRXrr      = 784,
RET_ReallyLR        = 791,
SEH_AddFP   = 805,
SEH_EpilogEnd       = 806,
SEH_EpilogStart     = 807,
SEH_Nop     = 808,
SEH_PrologEnd       = 809,
SEH_SaveFPLR        = 810,
SEH_SaveFPLR_X      = 811,
SEH_SaveFReg        = 812,
SEH_SaveFRegP       = 813,
SEH_SaveFRegP_X     = 814,
SEH_SaveFReg_X      = 815,
SEH_SaveReg = 816,
SEH_SaveRegP        = 817,
SEH_SaveRegP_X      = 818,
SEH_SaveReg_X       = 819,
SEH_SetFP   = 820,
SEH_StackAlloc      = 821,
SPACE       = 834,
TGloop
STGloop_wback       = 868,
STZGloop    = 872,
STZGloop_wback      = 873,
SUBSWrr     = 878,
SUBSXrr     = 879,
SUBWrr      = 880,
SUBXrr      = 881,
SpeculationBarrierISBDSBEndBB       = 892,
SpeculationBarrierSBEndBB   = 893,
SpeculationSafeValueW       = 894,
SpeculationSafeValueX       = 895,
StoreSwiftAsyncContext      = 896,
TAGPstack   = 897,
TCRETURNdi  = 898,
TCRETURNri  = 899,
TCRETURNriALL       = 900,
TCRETURNriBTI       = 901,
TLSDESCCALL = 902,
TLSDESC_CALLSEQ     = 903,

Most don't def a GPR32, and the ones that do (ADDWrr and friends, MOVi32imm) should be OK as far as I understand, as they always become W instructions.

[efriedma]

LGTM

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

218

Going through the list, I agree none of those are likely to be an issue. I'm a bit concerned about instructions that don't exist yet, but I guess we don't introduce pseudo-instructions very frequently, so maybe it isn't an issue.

[mstorsjo]

This triggers failed asserts for me:

$ cat repro.c
typedef int a;
typedef unsigned b;
int c, k, n;
b l;
void o();
typedef struct {
  a d, e;
  b f;
  b g;
  b h;
  b i
} j;
j m;
void p() {
  b q, r, s;
  for (;;) {
    if (c) {
      r = k - m.d;
      s = -m.e;
      q = -r;
    } else {
      r = m.h - m.f;
      s = m.i - m.g;
    }
    for (; l < s;) {
      o(r * sizeof(a));
      n = r + q;
    }
  }
}
$ clang -target aarch64-linux-gnu -c repro.c -O2
clang: ../lib/CodeGen/MachineRegisterInfo.cpp:399: llvm::MachineInstr* llvm::MachineRegisterInfo::getVRegDef(llvm::Register) const: Assertion `(I.atEnd() || std::next(I) == def_instr_end()) && "getVRegDef assumes a single definition or no definition"' failed.

[dmgreen]

In D127154#3565688, @mstorsjo wrote:

This triggers failed asserts for me:
...

Thanks for the reproducer - we are not preserving SSA in the intermediate steps, which is causing problems for subsequent combines. I feel like we should try and remove ToBeRemoved set, but that might require more testing to make sure it doesn't cause other problems.

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

if (N.getOpcode() == ISD::SHL) {
 
     Reg = N.getOperand(0).getOperand(0);
   } else {
     Ext = getExtendTypeForNode(N);
     if (Ext == AArch64_AM::InvalidShiftExtend)
       return false;
 
     Reg = N.getOperand(0);
 
-    // Don't match if free 32-bit -> 64-bit zext can be used instead.
-    if (Ext == AArch64_AM::UXTW &&
-        Reg->getValueType(0).getSizeInBits() == 32 && isDef32(*Reg.getNode()))
+    // Don't match if free 32-bit -> 64-bit zext can be used instead. Use the
+    // isDef32 as a heuristic for when the operand is likely to be a 32bit def.
+    auto isDef32 = [](SDValue N) {
+      unsigned Opc = N.getOpcode();
+      return Opc != ISD::TRUNCATE && Opc != TargetOpcode::EXTRACT_SUBREG &&
+             Opc != ISD::CopyFromReg && Opc != ISD::AssertSext &&
+             Opc != ISD::AssertZext && Opc != ISD::AssertAlign &&
+             Opc != ISD::FREEZE;
+    };
+    if (Ext == AArch64_AM::UXTW && Reg->getValueType(0).getSizeInBits() == 32 &&
+        isDef32(Reg))
       return false;
   }
 
   // AArch64 mandates that the RHS of the operation must use the smallest
   // register class that could contain the size being extended from.  Thus,
   // if we're folding a (sext i8), we need the RHS to be a GPR32, even though
   // there might not be an actual 32-bit value in the program.  We can
   // (harmlessly) synthesize one by injected an EXTRACT_SUBREG here.

llvm/lib/Target/AArch64/AArch64ISelLowering.h

enum NodeType : unsigned {
   MOPS_MEMSET,
   MOPS_MEMSET_TAGGING,
   MOPS_MEMCOPY,
   MOPS_MEMMOVE,
 };
 
 } // end namespace AArch64ISD
 
-namespace {
-
-// Any instruction that defines a 32-bit result zeros out the high half of the
-// register. Truncate can be lowered to EXTRACT_SUBREG. CopyFromReg may
-// be copying from a truncate. But any other 32-bit operation will zero-extend
-// up to 64 bits. AssertSext/AssertZext aren't saying anything about the upper
-// 32 bits, they're probably just qualifying a CopyFromReg.
-static inline bool isDef32(const SDNode &N) {
-  unsigned Opc = N.getOpcode();
-  return Opc != ISD::TRUNCATE && Opc != TargetOpcode::EXTRACT_SUBREG &&
-         Opc != ISD::CopyFromReg && Opc != ISD::AssertSext &&
-         Opc != ISD::AssertZext && Opc != ISD::AssertAlign &&
-         Opc != ISD::FREEZE;
-}
-
-} // end anonymous namespace
-
 namespace AArch64 {
 /// Possible values of current rounding mode, which is specified in bits
 /// 23:22 of FPCR.
 enum Rounding {
   RN = 0,    // Round to Nearest
   RP = 1,    // Round towards Plus infinity
   RM = 2,    // Round towards Minus infinity
   RZ = 3,    // Round towards Zero

llvm/lib/Target/AArch64/AArch64InstrInfo.td

 def SHA256SU0rr : SHATiedInstVV<0b0010, "sha256su0",int_aarch64_crypto_sha256su0>;
 }
 
 //----------------------------------------------------------------------------
 // Compiler-pseudos
 //----------------------------------------------------------------------------
 // FIXME: Like for X86, these should go in their own separate .td file.
 
-def def32 : PatLeaf<(i32 GPR32:$src), [{
-  return isDef32(*N);
-}]>;
-
-// In the case of a 32-bit def that is known to implicitly zero-extend,
-// we can use a SUBREG_TO_REG.
-def : Pat<(i64 (zext def32:$src)), (SUBREG_TO_REG (i64 0), GPR32:$src, sub_32)>;
-
 // For an anyext, we don't care what the high bits are, so we can perform an
 // INSERT_SUBREF into an IMPLICIT_DEF.
 def : Pat<(i64 (anyext GPR32:$src)),
           (INSERT_SUBREG (i64 (IMPLICIT_DEF)), GPR32:$src, sub_32)>;
 
 // When we need to explicitly zero-extend, we use a 32-bit MOV instruction and
 // then assert the extension has happened.
 def : Pat<(i64 (zext GPR32:$src)),

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

bool AArch64MIPeepholeOpt::visitORR(
   //
   // def : Pat<(i64 (zext GPR32:$src)),
   //           (SUBREG_TO_REG (i32 0), (ORRWrs WZR, GPR32:$src, 0), sub_32)>;
   if (MI.getOperand(3).getImm() != 0)
     return false;
 
   if (MI.getOperand(1).getReg() != AArch64::WZR)
     return false;
 
   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)
+  if (SrcMI->getOpcode() == TargetOpcode::COPY &&
+      SrcMI->getOperand(1).getReg().isVirtual()) {
+    const TargetRegisterClass *RC =
+        MRI->getRegClass(SrcMI->getOperand(1).getReg());
+
+    // A COPY from an FPR will become a FMOVSWr, so do so now so that we know
+    // that the upper bits are zero.
+    if (RC != &AArch64::FPR32RegClass &&
+        ((RC != &AArch64::FPR64RegClass && RC != &AArch64::FPR128RegClass) ||
+         SrcMI->getOperand(1).getSubReg() != AArch64::ssub))
+      return false;
+    Register CpySrc = SrcMI->getOperand(1).getReg();
+    if (SrcMI->getOperand(1).getSubReg() == AArch64::ssub) {
+      CpySrc = MRI->createVirtualRegister(&AArch64::FPR32RegClass);
+      BuildMI(*SrcMI->getParent(), SrcMI, SrcMI->getDebugLoc(),
+              TII->get(TargetOpcode::COPY), CpySrc)
+          .add(SrcMI->getOperand(1));
+    }
+    BuildMI(*SrcMI->getParent(), SrcMI, SrcMI->getDebugLoc(),
+            TII->get(AArch64::FMOVSWr), SrcMI->getOperand(0).getReg())
+        .addReg(CpySrc);
+    ToBeRemoved.insert(SrcMI);
+  }
+  else if (SrcMI->getOpcode() <= TargetOpcode::GENERIC_OP_END)
     return false;
 
   Register DefReg = MI.getOperand(0).getReg();
   Register SrcReg = MI.getOperand(2).getReg();
   MRI->replaceRegWith(DefReg, SrcReg);
   MRI->clearKillFlags(SrcReg);
   // replaceRegWith changes MI's definition register. Keep it for SSA form until
   // deleting MI.

llvm/test/CodeGen/AArch64/arm64-popcnt.ll

; CHECK-NONEON-NEXT:    ret
   %cnt = tail call i32 @llvm.ctpop.i32(i32 %x)
   ret i32 %cnt
 }
 
 define i32 @cnt32_advsimd_2(<2 x i32> %x) {
 ; CHECK-LABEL: cnt32_advsimd_2:
 ; CHECK:       // %bb.0:
 ; CHECK-NEXT:    // kill: def $d0 killed $d0 def $q0
-; CHECK-NEXT:    fmov w0, s0
-; CHECK-NEXT:    fmov d0, x0
+; CHECK-NEXT:    fmov w8, s0
+; CHECK-NEXT:    fmov d0, x8
 ; CHECK-NEXT:    cnt.8b v0, v0
 ; CHECK-NEXT:    uaddlv.8b h0, v0
 ; CHECK-NEXT:    fmov w0, s0
 ; CHECK-NEXT:    ret
 ;
 ; CHECK-NONEON-LABEL: cnt32_advsimd_2:
 ; CHECK-NONEON:       // %bb.0:
 ; CHECK-NONEON-NEXT:    lsr w9, w0, #1

llvm/test/CodeGen/AArch64/arm64-shifted-sext.ll

entry:
   %conv = sext i32 %inc to i64
   %shr = ashr i64 %conv, 32
   ret i64 %shr
 }
 
 define i64 @sign_extend_inreg_isdef32(i64) {
 ; CHECK-LABEL: sign_extend_inreg_isdef32:
 ; CHECK:       ; %bb.0:
-; CHECK-NEXT:    sbfx x0, x0, #32, #16
+; CHECK-NEXT:    sbfx x8, x0, #32, #16
+; CHECK-NEXT:    mov w0, w8
 ; CHECK-NEXT:    ret
   %2 = lshr i64 %0, 32
   %3 = shl i64 %2, 16
   %4 = trunc i64 %3 to i32
   %5 = ashr exact i32 %4, 16
   %6 = zext i32 %5 to i64
   ret i64 %6
 }

llvm/test/CodeGen/AArch64/dp1.ll

 ; CHECK-GISEL:       // %bb.0:
 ; CHECK-GISEL-NEXT:    adrp x8, :got:var64
 ; CHECK-GISEL-NEXT:    ldr x8, [x8, :got_lo12:var64]
 ; CHECK-GISEL-NEXT:    ldr x9, [x8]
 ; CHECK-GISEL-NEXT:    fmov d0, x9
 ; CHECK-GISEL-NEXT:    cnt v0.8b, v0.8b
 ; CHECK-GISEL-NEXT:    uaddlv h0, v0.8b
 ; CHECK-GISEL-NEXT:    fmov w9, s0
-; CHECK-GISEL-NEXT:    mov w9, w9
 ; CHECK-GISEL-NEXT:    str x9, [x8]
 ; CHECK-GISEL-NEXT:    ret
   %val0_tmp = load i64, i64* @var64
   %val4_tmp = call i64 @llvm.ctpop.i64(i64 %val0_tmp)
   store volatile i64 %val4_tmp, i64* @var64
   ret void
 }