[PATCH] D138187: [GlobalISel] Fix crash in applyShiftOfShiftedLogic caused by CSEMIRBuilder reuse instruction

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 created this revision.
bcl5980 added reviewers: arsenm, aemerson, paquette.
Herald added a subscriber: hiraditya.
Herald added a project: All.
bcl5980 requested review of this revision.
Herald added subscribers: llvm-commits, wdng.
Herald added a project: LLVM.

If LogicNonShiftReg is the same to Shift1Base, and shift1 const is the same to MatchInfo.Shift2 const, CSEMIRBuilder will reuse the old shift1 when build shift2. 
So, if we erase MatchInfo.Shift2 at the end, actually we remove old shift1. And it will cause crash later.

Solution for this issue is just erase it earlier to avoid the crash.

Fix #58432


https://reviews.llvm.org/D138187

Files:
  llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
  llvm/test/CodeGen/AArch64/pr58432.ll


Index: llvm/test/CodeGen/AArch64/pr58432.ll
===================================================================
--- /dev/null
+++ llvm/test/CodeGen/AArch64/pr58432.ll
@@ -0,0 +1,15 @@
+; 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
+
+; this used to crash
+define i32 @f(i32 %0) {
+; CHECK-LABEL: f:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsl w8, w0, #8
+; CHECK-NEXT:    orr w0, w8, w0, lsl #16
+; CHECK-NEXT:    ret
+  %2 = shl i32 %0, 8
+  %3 = or i32 %0, %2
+  %4 = shl i32 %3, 8
+  ret i32 %4
+}
Index: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
===================================================================
--- llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -1598,6 +1598,12 @@
   Register Shift1 =
       Builder.buildInstr(Opcode, {DestType}, {Shift1Base, Const}).getReg(0);
 
+  // If LogicNonShiftReg is the same to Shift1Base, and shift1 const is the same
+  // to MatchInfo.Shift2 const, CSEMIRBuilder will reuse the old shift1 when build 
+  // shift2. So, if we erase MatchInfo.Shift2 at the end, actually we remove old 
+  // shift1. And it will cause crash later. So erase it ealier to avoid the crash.
+  MatchInfo.Shift2->eraseFromParent();
+
   Register Shift2Const = MI.getOperand(2).getReg();
   Register Shift2 = Builder
                         .buildInstr(Opcode, {DestType},
@@ -1607,8 +1613,7 @@
   Register Dest = MI.getOperand(0).getReg();
   Builder.buildInstr(MatchInfo.Logic->getOpcode(), {Dest}, {Shift1, Shift2});
 
-  // These were one use so it's safe to remove them.
-  MatchInfo.Shift2->eraseFromParent();
+  // This was one use so it's safe to remove it.
   MatchInfo.Logic->eraseFromParent();
 
   MI.eraseFromParent();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D138187.476024.patch
Type: text/x-patch
Size: 1857 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221117/9a97f027/attachment.bin>