Skip to content

Commit a3aee26

Browse files
aemersonzmodem
authored andcommitted
[GlobalISel] Fix multiply with overflow intrinsics legalization generating invalid MIR.
During lowering of G_UMULO and friends, the previous code moved the builder's insertion point to be after the legalizing instruction. When that happened, if there happened to be a "G_CONSTANT i32 0" immediately after, the CSEMIRBuilder would try to find that constant during the buildConstant(zero) call, and since it dominates itself would return the iterator unchanged, even though the def of the constant was *after* the current insertion point. This resulted in the compare being generated *before* the constant which it was using. There's no need to modify the insertion point before building the mul-hi or constant. Delaying moving the insert point ensures those are built/CSEd before the G_ICMP is built. Fixes PR47679 Differential Revision: https://reviews.llvm.org/D88514 (cherry picked from commit 1d54e75)
1 parent dda0a18 commit a3aee26

File tree

4 files changed

+76
-11
lines changed

4 files changed

+76
-11
lines changed

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2368,11 +2368,12 @@ LegalizerHelper::lower(MachineInstr &MI, unsigned TypeIdx, LLT Ty) {
23682368
MI.RemoveOperand(1);
23692369
Observer.changedInstr(MI);
23702370

2371-
MIRBuilder.setInsertPt(MIRBuilder.getMBB(), ++MIRBuilder.getInsertPt());
2372-
23732371
auto HiPart = MIRBuilder.buildInstr(Opcode, {Ty}, {LHS, RHS});
23742372
auto Zero = MIRBuilder.buildConstant(Ty, 0);
23752373

2374+
// Move insert point forward so we can use the Res register if needed.
2375+
MIRBuilder.setInsertPt(MIRBuilder.getMBB(), ++MIRBuilder.getInsertPt());
2376+
23762377
// For *signed* multiply, overflow is detected by checking:
23772378
// (hi != (lo >> bitwidth-1))
23782379
if (Opcode == TargetOpcode::G_SMULH) {

llvm/test/CodeGen/AArch64/GlobalISel/legalize-mul.mir

Lines changed: 66 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ body: |
2828
; CHECK-LABEL: name: test_smul_overflow
2929
; CHECK: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
3030
; CHECK: [[COPY1:%[0-9]+]]:_(s64) = COPY $x1
31-
; CHECK: [[MUL:%[0-9]+]]:_(s64) = G_MUL [[COPY]], [[COPY1]]
3231
; CHECK: [[SMULH:%[0-9]+]]:_(s64) = G_SMULH [[COPY]], [[COPY1]]
32+
; CHECK: [[MUL:%[0-9]+]]:_(s64) = G_MUL [[COPY]], [[COPY1]]
3333
; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 63
3434
; CHECK: [[ASHR:%[0-9]+]]:_(s64) = G_ASHR [[MUL]], [[C]]
3535
; CHECK: [[ICMP:%[0-9]+]]:_(s32) = G_ICMP intpred(ne), [[SMULH]](s64), [[ASHR]]
@@ -51,9 +51,9 @@ body: |
5151
; CHECK-LABEL: name: test_umul_overflow
5252
; CHECK: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
5353
; CHECK: [[COPY1:%[0-9]+]]:_(s64) = COPY $x1
54-
; CHECK: [[MUL:%[0-9]+]]:_(s64) = G_MUL [[COPY]], [[COPY1]]
5554
; CHECK: [[UMULH:%[0-9]+]]:_(s64) = G_UMULH [[COPY]], [[COPY1]]
5655
; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
56+
; CHECK: [[MUL:%[0-9]+]]:_(s64) = G_MUL [[COPY]], [[COPY1]]
5757
; CHECK: [[ICMP:%[0-9]+]]:_(s32) = G_ICMP intpred(ne), [[UMULH]](s64), [[C]]
5858
; CHECK: $x0 = COPY [[MUL]](s64)
5959
; CHECK: [[COPY2:%[0-9]+]]:_(s32) = COPY [[ICMP]](s32)
@@ -66,3 +66,67 @@ body: |
6666
$w0 = COPY %4(s32)
6767
6868
...
69+
---
70+
name: test_umulo_overflow_no_invalid_mir
71+
alignment: 4
72+
tracksRegLiveness: true
73+
liveins:
74+
- { reg: '$x0' }
75+
- { reg: '$x1' }
76+
- { reg: '$x2' }
77+
frameInfo:
78+
maxAlignment: 16
79+
stack:
80+
- { id: 0, size: 8, alignment: 8 }
81+
- { id: 1, size: 8, alignment: 8 }
82+
- { id: 2, size: 16, alignment: 16 }
83+
- { id: 3, size: 16, alignment: 8 }
84+
machineFunctionInfo: {}
85+
body: |
86+
bb.1:
87+
liveins: $x0, $x1, $x2
88+
; Check that the overflow result doesn't generate incorrect MIR by using a G_CONSTANT 0
89+
; before it's been defined.
90+
; CHECK-LABEL: name: test_umulo_overflow_no_invalid_mir
91+
; CHECK: liveins: $x0, $x1, $x2
92+
; CHECK: [[COPY:%[0-9]+]]:_(p0) = COPY $x0
93+
; CHECK: [[COPY1:%[0-9]+]]:_(s64) = COPY $x1
94+
; CHECK: [[COPY2:%[0-9]+]]:_(s64) = COPY $x2
95+
; CHECK: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.0
96+
; CHECK: [[FRAME_INDEX1:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.1
97+
; CHECK: [[FRAME_INDEX2:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.3
98+
; CHECK: G_STORE [[COPY2]](s64), [[FRAME_INDEX]](p0) :: (store 8)
99+
; CHECK: G_STORE [[COPY1]](s64), [[FRAME_INDEX1]](p0) :: (store 8)
100+
; CHECK: [[LOAD:%[0-9]+]]:_(s64) = G_LOAD [[FRAME_INDEX]](p0) :: (dereferenceable load 8)
101+
; CHECK: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[FRAME_INDEX1]](p0) :: (dereferenceable load 8)
102+
; CHECK: [[UMULH:%[0-9]+]]:_(s64) = G_UMULH [[LOAD]], [[LOAD1]]
103+
; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
104+
; CHECK: [[MUL:%[0-9]+]]:_(s64) = G_MUL [[LOAD]], [[LOAD1]]
105+
; CHECK: [[ICMP:%[0-9]+]]:_(s32) = G_ICMP intpred(ne), [[UMULH]](s64), [[C]]
106+
; CHECK: G_STORE [[C]](s64), [[FRAME_INDEX2]](p0) :: (store 8, align 1)
107+
; CHECK: [[C1:%[0-9]+]]:_(s64) = G_CONSTANT i64 1
108+
; CHECK: [[ANYEXT:%[0-9]+]]:_(s64) = G_ANYEXT [[ICMP]](s32)
109+
; CHECK: [[AND:%[0-9]+]]:_(s64) = G_AND [[ANYEXT]], [[C1]]
110+
; CHECK: $x0 = COPY [[MUL]](s64)
111+
; CHECK: $x1 = COPY [[AND]](s64)
112+
; CHECK: RET_ReallyLR implicit $x0
113+
%0:_(p0) = COPY $x0
114+
%1:_(s64) = COPY $x1
115+
%2:_(s64) = COPY $x2
116+
%25:_(s32) = G_CONSTANT i32 0
117+
%3:_(p0) = G_FRAME_INDEX %stack.0
118+
%4:_(p0) = G_FRAME_INDEX %stack.1
119+
%6:_(p0) = G_FRAME_INDEX %stack.3
120+
G_STORE %2(s64), %3(p0) :: (store 8)
121+
G_STORE %1(s64), %4(p0) :: (store 8)
122+
%7:_(s64) = G_LOAD %3(p0) :: (dereferenceable load 8)
123+
%8:_(s64) = G_LOAD %4(p0) :: (dereferenceable load 8)
124+
%9:_(s64), %10:_(s1) = G_UMULO %7, %8
125+
%31:_(s64) = G_CONSTANT i64 0
126+
G_STORE %31(s64), %6(p0) :: (store 8, align 1)
127+
%16:_(s64) = G_ZEXT %10(s1)
128+
$x0 = COPY %9(s64)
129+
$x1 = COPY %16(s64)
130+
RET_ReallyLR implicit $x0
131+
132+
...

llvm/test/CodeGen/Mips/GlobalISel/legalizer/mul.mir

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -439,9 +439,9 @@ body: |
439439
; MIPS32: [[COPY1:%[0-9]+]]:_(s32) = COPY $a1
440440
; MIPS32: [[COPY2:%[0-9]+]]:_(p0) = COPY $a2
441441
; MIPS32: [[COPY3:%[0-9]+]]:_(p0) = COPY $a3
442-
; MIPS32: [[MUL:%[0-9]+]]:_(s32) = G_MUL [[COPY]], [[COPY1]]
443442
; MIPS32: [[UMULH:%[0-9]+]]:_(s32) = G_UMULH [[COPY]], [[COPY1]]
444443
; MIPS32: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 0
444+
; MIPS32: [[MUL:%[0-9]+]]:_(s32) = G_MUL [[COPY]], [[COPY1]]
445445
; MIPS32: [[ICMP:%[0-9]+]]:_(s32) = G_ICMP intpred(ne), [[UMULH]](s32), [[C]]
446446
; MIPS32: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 1
447447
; MIPS32: [[COPY4:%[0-9]+]]:_(s32) = COPY [[ICMP]](s32)

llvm/test/CodeGen/Mips/GlobalISel/llvm-ir/mul.ll

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -180,13 +180,13 @@ declare { i32, i1 } @llvm.umul.with.overflow.i32(i32, i32)
180180
define void @umul_with_overflow(i32 %lhs, i32 %rhs, i32* %pmul, i1* %pcarry_flag) {
181181
; MIPS32-LABEL: umul_with_overflow:
182182
; MIPS32: # %bb.0:
183-
; MIPS32-NEXT: mul $1, $4, $5
184183
; MIPS32-NEXT: multu $4, $5
185-
; MIPS32-NEXT: mfhi $2
186-
; MIPS32-NEXT: sltu $2, $zero, $2
187-
; MIPS32-NEXT: andi $2, $2, 1
188-
; MIPS32-NEXT: sb $2, 0($7)
189-
; MIPS32-NEXT: sw $1, 0($6)
184+
; MIPS32-NEXT: mfhi $1
185+
; MIPS32-NEXT: mul $2, $4, $5
186+
; MIPS32-NEXT: sltu $1, $zero, $1
187+
; MIPS32-NEXT: andi $1, $1, 1
188+
; MIPS32-NEXT: sb $1, 0($7)
189+
; MIPS32-NEXT: sw $2, 0($6)
190190
; MIPS32-NEXT: jr $ra
191191
; MIPS32-NEXT: nop
192192
%res = call { i32, i1 } @llvm.umul.with.overflow.i32(i32 %lhs, i32 %rhs)

0 commit comments

Comments
 (0)