[PATCH] D104868: [ARM] Fix crash in chained BFI combine due to incorrectly RAUW'ing a node.

[Amara Emerson via Phabricator via llvm-commits]

aemerson created this revision.
aemerson added reviewers: t.p.northover, jmolloy, efriedma.
aemerson added a project: LLVM.
Herald added subscribers: danielkiss, hiraditya, kristof.beyls.
aemerson requested review of this revision.

For a bfi chain like:

  a = bfi input, x, y
  b = bfi a, x', y'

The previous code was RAUW'ing `a` with `x`, mutating the second `b` bfi, and when
SelectionDAG's CSE code ended up deleting it unexpectedly, bad things happend.
There's no need to RAUW in this case because we can just return our newly
created replacement BFI node. It also looked incorrect because it didn't account
for other users of the `a` bfi.

rdar://79095399


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104868

Files:
  llvm/lib/Target/ARM/ARMISelLowering.cpp
  llvm/test/CodeGen/ARM/bfi-chain-cse-crash.ll


Index: llvm/test/CodeGen/ARM/bfi-chain-cse-crash.ll
===================================================================
--- /dev/null
+++ llvm/test/CodeGen/ARM/bfi-chain-cse-crash.ll
@@ -0,0 +1,46 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=thumbv7s | FileCheck %s
+target datalayout = "e-m:o-p:32:32-f64:32:64-v64:32:64-v128:32:128-a:0:32-n32-S32"
+target triple = "thumbv7s-apple-ios3.1.3"
+
+define void @bfi_chain_cse_crash(i8* %0) {
+; CHECK-LABEL: bfi_chain_cse_crash:
+; CHECK:       @ %bb.0: @ %entry
+; CHECK-NEXT:    ldrb r1, [r0]
+; CHECK-NEXT:    and r2, r1, #1
+; CHECK-NEXT:    lsrs r1, r1, #3
+; CHECK-NEXT:    bfi r2, r1, #3, #1
+; CHECK-NEXT:    strb r2, [r0]
+; CHECK-NEXT:    movs r0, #0
+; CHECK-NEXT:    cmp r0, #0
+; CHECK-NEXT:    bx lr
+entry:
+  %1 = load i8, i8* undef, align 1
+  %2 = and i8 %1, 1
+  %3 = select i1 false, i8 %2, i8 undef
+  %4 = and i8 %1, 4
+  %5 = icmp eq i8 %4, 0
+  %6 = zext i8 %3 to i32
+  %7 = or i32 %6, 4
+  %8 = trunc i32 %7 to i8
+  %9 = select i1 %5, i8 %3, i8 %8
+  %10 = and i8 %1, 8
+  %11 = icmp eq i8 %10, 0
+  %12 = zext i8 %2 to i32
+  %13 = or i32 %12, 8
+  %14 = trunc i32 %13 to i8
+  %15 = zext i8 %9 to i32
+  %16 = or i32 %15, 8
+  %17 = trunc i32 %16 to i8
+  %18 = select i1 %11, i8 %2, i8 %14
+  %19 = select i1 %11, i8 %9, i8 %17
+  store i8 %18, i8* %0, align 1
+  br i1 undef, label %.preheader, label %.loopexit
+
+.preheader:                                       ; preds = %.loopexit22
+  %. = select i1 false, i8 undef, i8 %19
+  br label %.loopexit
+
+.loopexit:                                        ; preds = %.preheader, %.loopexit22
+  ret void
+}
Index: llvm/lib/Target/ARM/ARMISelLowering.cpp
===================================================================
--- llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -14035,9 +14035,7 @@
     assert(From1 == From2);
     (void)From2;
 
-    // First, unlink CombineBFI.
-    DCI.DAG.ReplaceAllUsesWith(CombineBFI, CombineBFI.getOperand(0));
-    // Then create a new BFI, combining the two together.
+    // Create a new BFI, combining the two together.
     APInt NewFromMask = FromMask1 | FromMask2;
     APInt NewToMask = ToMask1 | ToMask2;
 
@@ -14046,9 +14044,9 @@
 
     if (NewFromMask[0] == 0)
       From1 = DCI.DAG.getNode(
-        ISD::SRL, dl, VT, From1,
-        DCI.DAG.getConstant(NewFromMask.countTrailingZeros(), dl, VT));
-    return DCI.DAG.getNode(ARMISD::BFI, dl, VT, N->getOperand(0), From1,
+          ISD::SRL, dl, VT, From1,
+          DCI.DAG.getConstant(NewFromMask.countTrailingZeros(), dl, VT));
+    return DCI.DAG.getNode(ARMISD::BFI, dl, VT, CombineBFI.getOperand(0), From1,
                            DCI.DAG.getConstant(~NewToMask, dl, VT));
   }
   return SDValue();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D104868.354311.patch
Type: text/x-patch
Size: 2842 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210624/72effae1/attachment.bin>