[GlobalISel][AArch64] Fix miscompile caused by wrong G_ZEXT selection in GISel

[bcl5980]

Summary

The miscompile case's G_ZEXT has a G_FREEZE source. Similar to D127154, this patch removed isDef32, relying on the AArch64MIPeephole optimizer to remove redundant SUBREG_TO_REG nodes also in GISel.

Fix #58431

[bcl5980]

Update test for the case.

[bcl5980]

Need to fix tests failure.

[efriedma]

I think the bug is that GlobalISel is emitting SUBREG_TO_REG, not that it's getting eliminated later. See https://reviews.llvm.org/D127154 .

[bcl5980]

Update by @efriedma 's comment.

[efriedma]

Maybe GlobalISel should stop using isDef32, like we did for SelectionDAG? I mean, this fix isn't really wrong, but I think we have the same general issue as SelectionDAG; it's hard to tell how a generic operation is going to be lowered before it's actually lowered.

[dmgreen]

Yeah I agree. I don't have anything setup to test Global ISel, and we don't have the same breadth of lit tests, but I hope that it is similar to Selection DAG where the later peephole optimizations are enough to remove the extra instructions.

[bcl5980]

@efriedma @dmgreen, do we need to add G_FREEZE into AArch64InstructionSelector::isDef32 for AArch64InstructionSelector::selectArithExtendedRegister to match SelectionDAG?

[bcl5980]

Update tests.

[efriedma]

I don't think the usage in selectArithExtendedRegister actually needs to work reliably for correctness; the pattern doesn't assume anything about the high bits, and if we don't use the pattern, we just fallback to a normal zext. So it doesn't really matter that much. (See also the SelectionDAG version of this check in AArch64DAGToDAGISel::SelectArithExtendedRegister.) Ideally, I guess it would look through freeze instructions. Or maybe we should just never match "uxtw" during isel, and just add something to AArch64MIPeepholeOpt. In any case, let's fix the miscompile, and leave it for later.

Patch looks fine, but this should be approved by one of the AArch64 GlobalISel maintainers.

[paquette]

This looks fine to me.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

if (!IsSigned) {
         const MachineMemOperand *MemOp = *LoadMI->memoperands_begin();
         unsigned BytesLoaded = MemOp->getSize();
         if (BytesLoaded < 4 && SrcTy.getSizeInBytes() == BytesLoaded)
           return selectCopy(I, TII, MRI, TRI, RBI);
       }
 
       // For the 32-bit -> 64-bit case, we can emit a mov (ORRWrs)
       // + SUBREG_TO_REG.
-      //
-      // If we are zero extending from 32 bits to 64 bits, it's possible that
-      // the instruction implicitly does the zero extend for us. In that case,
-      // we only need the SUBREG_TO_REG.
       if (IsGPR && SrcSize == 32 && DstSize == 64) {
-        // Unlike with the G_LOAD case, we don't want to look through copies
-        // here. (See isDef32.)
-        MachineInstr *Def = MRI.getVRegDef(SrcReg);
-        Register SubregToRegSrc = SrcReg;
-
-        // Does the instruction implicitly zero extend?
-        if (!Def || !isDef32(*Def)) {
-          // No. Zero out using an OR.
-          Register OrDst = MRI.createVirtualRegister(&AArch64::GPR32RegClass);
-          const Register ZReg = AArch64::WZR;
-          MIB.buildInstr(AArch64::ORRWrs, {OrDst}, {ZReg, SrcReg}).addImm(0);
-          SubregToRegSrc = OrDst;
-        }
+        Register SubregToRegSrc =
+            MRI.createVirtualRegister(&AArch64::GPR32RegClass);
+        MIB.buildInstr(AArch64::ORRWrs, {SubregToRegSrc}, {ZReg, SrcReg})
+            .addImm(0);
 
         MIB.buildInstr(AArch64::SUBREG_TO_REG, {DefReg}, {})
             .addImm(0)
             .addUse(SubregToRegSrc)
             .addImm(AArch64::sub_32);
 
         if (!RBI.constrainGenericRegister(DefReg, AArch64::GPR64RegClass,
                                           MRI)) {

llvm/test/CodeGen/AArch64/pr58431.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=aarch64-none-linux-gnu -global-isel -global-isel-abort=0 | FileCheck %s
+
+define i32 @f(i64 %0) {
+; CHECK-LABEL: f:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    mov w8, #10
+; CHECK-NEXT:    mov w9, w0
+; CHECK-NEXT:    udiv x10, x9, x8
+; CHECK-NEXT:    msub x0, x10, x8, x9
+; CHECK-NEXT:    // kill: def $w0 killed $w0 killed $x0
+; CHECK-NEXT:    ret
+  %2 = trunc i64 %0 to i32
+  %3 = freeze i32 %2
+  %4 = zext i32 %3 to i64
+  %5 = urem i64 %4, 10
+  %6 = trunc i64 %5 to i32
+  ret i32 %6
+}