[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/AArch64RegisterInfo.cpp

bool AArch64RegisterInfo::shouldCoalesce(
   if (MI->isCopy() &&
       ((DstRC->getID() == AArch64::GPR64RegClassID) ||
        (DstRC->getID() == AArch64::GPR64commonRegClassID)) &&
       MI->getOperand(0).getSubReg() && MI->getOperand(1).getSubReg())
     // Do not coalesce in the case of a 32-bit subregister copy
     // which implements a 32 to 64 bit zero extension
     // which relies on the upper 32 bits being zeroed.
     return false;
+
+  // Similar to 32-bit subregister copy, subreg_to_reg also
+  // need to disable coalesce
+  if (MI->isSubregToReg() &&
+      ((DstRC->getID() == AArch64::GPR64RegClassID) ||
+       (DstRC->getID() == AArch64::GPR64commonRegClassID)) &&
+      MI->getOperand(1).getImm() == 0 &&
+      MI->getOperand(3).getImm() == AArch64::sub_32)
+    return false;
   return true;
 }

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 w9, w0
+; CHECK-NEXT:    mov w8, #10
+; CHECK-NEXT:    // kill: def $x8 killed $w8
+; 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
+}