Skip to content

Commit 1d54e75

Browse files
committed
[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
1 parent 618a890 commit 1d54e75

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
@@ -2892,11 +2892,12 @@ LegalizerHelper::lower(MachineInstr &MI, unsigned TypeIdx, LLT LowerHintTy) {
28922892
MI.RemoveOperand(1);
28932893
Observer.changedInstr(MI);
28942894

2895-
MIRBuilder.setInsertPt(MIRBuilder.getMBB(), ++MIRBuilder.getInsertPt());
2896-
28972895
auto HiPart = MIRBuilder.buildInstr(Opcode, {Ty}, {LHS, RHS});
28982896
auto Zero = MIRBuilder.buildConstant(Ty, 0);
28992897

2898+
// Move insert point forward so we can use the Res register if needed.
2899+
MIRBuilder.setInsertPt(MIRBuilder.getMBB(), ++MIRBuilder.getInsertPt());
2900+
29002901
// For *signed* multiply, overflow is detected by checking:
29012902
// (hi != (lo >> bitwidth-1))
29022903
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]](s64)
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)
@@ -91,3 +91,67 @@ body: |
9191
$q0 = COPY %2(<2 x s64>)
9292
RET_ReallyLR implicit $q0
9393
...
94+
---
95+
name: test_umulo_overflow_no_invalid_mir
96+
alignment: 4
97+
tracksRegLiveness: true
98+
liveins:
99+
- { reg: '$x0' }
100+
- { reg: '$x1' }
101+
- { reg: '$x2' }
102+
frameInfo:
103+
maxAlignment: 16
104+
stack:
105+
- { id: 0, size: 8, alignment: 8 }
106+
- { id: 1, size: 8, alignment: 8 }
107+
- { id: 2, size: 16, alignment: 16 }
108+
- { id: 3, size: 16, alignment: 8 }
109+
machineFunctionInfo: {}
110+
body: |
111+
bb.1:
112+
liveins: $x0, $x1, $x2
113+
; Check that the overflow result doesn't generate incorrect MIR by using a G_CONSTANT 0
114+
; before it's been defined.
115+
; CHECK-LABEL: name: test_umulo_overflow_no_invalid_mir
116+
; CHECK: liveins: $x0, $x1, $x2
117+
; CHECK: [[COPY:%[0-9]+]]:_(p0) = COPY $x0
118+
; CHECK: [[COPY1:%[0-9]+]]:_(s64) = COPY $x1
119+
; CHECK: [[COPY2:%[0-9]+]]:_(s64) = COPY $x2
120+
; CHECK: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.0
121+
; CHECK: [[FRAME_INDEX1:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.1
122+
; CHECK: [[FRAME_INDEX2:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.3
123+
; CHECK: G_STORE [[COPY2]](s64), [[FRAME_INDEX]](p0) :: (store 8)
124+
; CHECK: G_STORE [[COPY1]](s64), [[FRAME_INDEX1]](p0) :: (store 8)
125+
; CHECK: [[LOAD:%[0-9]+]]:_(s64) = G_LOAD [[FRAME_INDEX]](p0) :: (dereferenceable load 8)
126+
; CHECK: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[FRAME_INDEX1]](p0) :: (dereferenceable load 8)
127+
; CHECK: [[UMULH:%[0-9]+]]:_(s64) = G_UMULH [[LOAD]], [[LOAD1]]
128+
; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
129+
; CHECK: [[MUL:%[0-9]+]]:_(s64) = G_MUL [[LOAD]], [[LOAD1]]
130+
; CHECK: [[ICMP:%[0-9]+]]:_(s32) = G_ICMP intpred(ne), [[UMULH]](s64), [[C]]
131+
; CHECK: G_STORE [[C]](s64), [[FRAME_INDEX2]](p0) :: (store 8, align 1)
132+
; CHECK: [[C1:%[0-9]+]]:_(s64) = G_CONSTANT i64 1
133+
; CHECK: [[ANYEXT:%[0-9]+]]:_(s64) = G_ANYEXT [[ICMP]](s32)
134+
; CHECK: [[AND:%[0-9]+]]:_(s64) = G_AND [[ANYEXT]], [[C1]]
135+
; CHECK: $x0 = COPY [[MUL]](s64)
136+
; CHECK: $x1 = COPY [[AND]](s64)
137+
; CHECK: RET_ReallyLR implicit $x0
138+
%0:_(p0) = COPY $x0
139+
%1:_(s64) = COPY $x1
140+
%2:_(s64) = COPY $x2
141+
%25:_(s32) = G_CONSTANT i32 0
142+
%3:_(p0) = G_FRAME_INDEX %stack.0
143+
%4:_(p0) = G_FRAME_INDEX %stack.1
144+
%6:_(p0) = G_FRAME_INDEX %stack.3
145+
G_STORE %2(s64), %3(p0) :: (store 8)
146+
G_STORE %1(s64), %4(p0) :: (store 8)
147+
%7:_(s64) = G_LOAD %3(p0) :: (dereferenceable load 8)
148+
%8:_(s64) = G_LOAD %4(p0) :: (dereferenceable load 8)
149+
%9:_(s64), %10:_(s1) = G_UMULO %7, %8
150+
%31:_(s64) = G_CONSTANT i64 0
151+
G_STORE %31(s64), %6(p0) :: (store 8, align 1)
152+
%16:_(s64) = G_ZEXT %10(s1)
153+
$x0 = COPY %9(s64)
154+
$x1 = COPY %16(s64)
155+
RET_ReallyLR implicit $x0
156+
157+
...

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)