Skip to content

Commit b2d3f2e

Browse files
ramntryramntry
committed
[MIR][MachineCSE] Implementing proper MachineInstr::getNumExplicitDefs()
Apparently, MachineInstr class definition as well as pretty much all of the machine passes assume that the only kind of MachineInstr's operands that is variadic for variadic opcodes is explicit non-definitions. In particular, this assumption is made by MachineInstr::defs(), uses(), and explicit_uses() methods, as well as by MachineCSE pass. The assumption is incorrect judging from at least TableGen backend implementation, that recognizes variable_ops in OutOperandList, and the very existence of G_UNMERGE_VALUES generic opcode, or ARM load multiple instructions, all of which have variadic defs. In particular, MachineCSE pass breaks MIR with CSE'able G_UNMERGE_VALUES instructions in it. This commit implements MachineInstr::getNumExplicitDefs() similar to pre-existing MachineInstr::getNumExplicitOperands(), fixes MachineInstr::defs(), uses(), and explicit_uses(), and fixes MachineCSE pass. As the issue addressed seems to affect only machine passes that could be ran mid-GlobalISel pipeline at the moment, the other passes aren't fixed by this commit, like MachineLICM: that could be done on per-pass basis when (if ever) they get adopted for GlobalISel. Reviewed By: arsenm Differential Revision: https://reviews.llvm.org/D45640 llvm-svn: 334520
1 parent 00f2cb1 commit b2d3f2e

File tree

4 files changed

+140
-18
lines changed

4 files changed

+140
-18
lines changed

‎llvm/include/llvm/CodeGen/MachineInstr.h

+16-10
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,11 @@ class MachineInstr
322322
return Operands[i];
323323
}
324324

325+
/// Returns the total number of definitions.
326+
unsigned getNumDefs() const {
327+
return getNumExplicitDefs() + MCID->getNumImplicitDefs();
328+
}
329+
325330
/// Return true if operand \p OpIdx is a subregister index.
326331
bool isOperandSubregIdx(unsigned OpIdx) const {
327332
assert(getOperand(OpIdx).getType() == MachineOperand::MO_Immediate &&
@@ -340,6 +345,9 @@ class MachineInstr
340345
/// Returns the number of non-implicit operands.
341346
unsigned getNumExplicitOperands() const;
342347

348+
/// Returns the number of non-implicit definitions.
349+
unsigned getNumExplicitDefs() const;
350+
343351
/// iterator/begin/end - Iterate over all operands of a machine instruction.
344352
using mop_iterator = MachineOperand *;
345353
using const_mop_iterator = const MachineOperand *;
@@ -374,31 +382,29 @@ class MachineInstr
374382
/// Implicit definition are not included!
375383
iterator_range<mop_iterator> defs() {
376384
return make_range(operands_begin(),
377-
operands_begin() + getDesc().getNumDefs());
385+
operands_begin() + getNumExplicitDefs());
378386
}
379387
/// \copydoc defs()
380388
iterator_range<const_mop_iterator> defs() const {
381389
return make_range(operands_begin(),
382-
operands_begin() + getDesc().getNumDefs());
390+
operands_begin() + getNumExplicitDefs());
383391
}
384392
/// Returns a range that includes all operands that are register uses.
385393
/// This may include unrelated operands which are not register uses.
386394
iterator_range<mop_iterator> uses() {
387-
return make_range(operands_begin() + getDesc().getNumDefs(),
388-
operands_end());
395+
return make_range(operands_begin() + getNumExplicitDefs(), operands_end());
389396
}
390397
/// \copydoc uses()
391398
iterator_range<const_mop_iterator> uses() const {
392-
return make_range(operands_begin() + getDesc().getNumDefs(),
393-
operands_end());
399+
return make_range(operands_begin() + getNumExplicitDefs(), operands_end());
394400
}
395401
iterator_range<mop_iterator> explicit_uses() {
396-
return make_range(operands_begin() + getDesc().getNumDefs(),
397-
operands_begin() + getNumExplicitOperands() );
402+
return make_range(operands_begin() + getNumExplicitDefs(),
403+
operands_begin() + getNumExplicitOperands());
398404
}
399405
iterator_range<const_mop_iterator> explicit_uses() const {
400-
return make_range(operands_begin() + getDesc().getNumDefs(),
401-
operands_begin() + getNumExplicitOperands() );
406+
return make_range(operands_begin() + getNumExplicitDefs(),
407+
operands_begin() + getNumExplicitOperands());
402408
}
403409

404410
/// Returns the number of the operand iterator \p I points to.

‎llvm/lib/CodeGen/MachineCSE.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -550,8 +550,7 @@ bool MachineCSE::ProcessBlock(MachineBasicBlock *MBB) {
550550

551551
// Check if it's profitable to perform this CSE.
552552
bool DoCSE = true;
553-
unsigned NumDefs = MI->getDesc().getNumDefs() +
554-
MI->getDesc().getNumImplicitDefs();
553+
unsigned NumDefs = MI->getNumDefs();
555554

556555
for (unsigned i = 0, e = MI->getNumOperands(); NumDefs && i != e; ++i) {
557556
MachineOperand &MO = MI->getOperand(i);

‎llvm/lib/CodeGen/MachineInstr.cpp

+24-6
Original file line numberDiff line numberDiff line change
@@ -519,21 +519,39 @@ void MachineInstr::eraseFromBundle() {
519519
getParent()->erase_instr(this);
520520
}
521521

522-
/// getNumExplicitOperands - Returns the number of non-implicit operands.
523-
///
524522
unsigned MachineInstr::getNumExplicitOperands() const {
525523
unsigned NumOperands = MCID->getNumOperands();
526524
if (!MCID->isVariadic())
527525
return NumOperands;
528526

529-
for (unsigned i = NumOperands, e = getNumOperands(); i != e; ++i) {
530-
const MachineOperand &MO = getOperand(i);
531-
if (!MO.isReg() || !MO.isImplicit())
532-
NumOperands++;
527+
for (unsigned I = NumOperands, E = getNumOperands(); I != E; ++I) {
528+
const MachineOperand &MO = getOperand(I);
529+
// The operands must always be in the following order:
530+
// - explicit reg defs,
531+
// - other explicit operands (reg uses, immediates, etc.),
532+
// - implicit reg defs
533+
// - implicit reg uses
534+
if (MO.isReg() && MO.isImplicit())
535+
break;
536+
++NumOperands;
533537
}
534538
return NumOperands;
535539
}
536540

541+
unsigned MachineInstr::getNumExplicitDefs() const {
542+
unsigned NumDefs = MCID->getNumDefs();
543+
if (!MCID->isVariadic())
544+
return NumDefs;
545+
546+
for (unsigned I = NumDefs, E = getNumOperands(); I != E; ++I) {
547+
const MachineOperand &MO = getOperand(I);
548+
if (!MO.isReg() || !MO.isDef() || MO.isImplicit())
549+
break;
550+
++NumDefs;
551+
}
552+
return NumDefs;
553+
}
554+
537555
void MachineInstr::bundleWithPred() {
538556
assert(!isBundledWithPred() && "MI is already bundled with its predecessor");
539557
setFlag(BundledPred);

‎llvm/test/CodeGen/AArch64/GlobalISel/machine-cse-mid-pipeline.mir

+99
Original file line numberDiff line numberDiff line change
@@ -179,3 +179,102 @@ body: |
179179
$w0 = COPY %4
180180
RET_ReallyLR implicit $w0
181181
...
182+
---
183+
name: variadic_defs_unmerge_vector
184+
legalized: true
185+
regBankSelected: false
186+
selected: false
187+
body: |
188+
; CHECK-LABEL: name: variadic_defs_unmerge_vector
189+
; CHECK: [[COPY:%[0-9]+]]:_(<4 x s16>) = COPY $d0
190+
; CHECK-NEXT: [[UV0:%[0-9]+]]:_(s16), [[UV1:%[0-9]+]]:_(s16), [[UV2:%[0-9]+]]:_(s16), [[UV3:%[0-9]+]]:_(s16) = G_UNMERGE_VALUES [[COPY]](<4 x s16>)
191+
; CHECK-NEXT: [[ANYEXT0:%[0-9]+]]:_(s32) = G_ANYEXT [[UV0]](s16)
192+
; CHECK-NEXT: [[ANYEXT1:%[0-9]+]]:_(s32) = G_ANYEXT [[UV1]](s16)
193+
; CHECK-NEXT: [[ANYEXT2:%[0-9]+]]:_(s32) = G_ANYEXT [[UV2]](s16)
194+
; CHECK-NEXT: [[ANYEXT3:%[0-9]+]]:_(s32) = G_ANYEXT [[UV3]](s16)
195+
; CHECK-NEXT: [[ADD0:%[0-9]+]]:_(s32) = G_ADD [[ANYEXT0]], [[ANYEXT1]]
196+
; CHECK-NEXT: [[ADD1:%[0-9]+]]:_(s32) = G_ADD [[ANYEXT2]], [[ANYEXT3]]
197+
; CHECK-NEXT: [[ADD2:%[0-9]+]]:_(s32) = G_ADD [[ADD0]], [[ADD1]]
198+
; CHECK-NEXT: $w0 = COPY [[ADD2]](s32)
199+
; CHECK-NEXT: RET_ReallyLR implicit $w0
200+
bb.0:
201+
%0 :_(<4 x s16>) = COPY $d0
202+
%1 :_(s16), %2 :_(s16), %3 :_(s16), %4 :_(s16) = G_UNMERGE_VALUES %0(<4 x s16>)
203+
%5 :_(s16), %6 :_(s16), %7 :_(s16), %8 :_(s16) = G_UNMERGE_VALUES %0(<4 x s16>)
204+
%9 :_(s16), %10:_(s16), %11:_(s16), %12:_(s16) = G_UNMERGE_VALUES %0(<4 x s16>)
205+
%13:_(s16), %14:_(s16), %15:_(s16), %16:_(s16) = G_UNMERGE_VALUES %0(<4 x s16>)
206+
%17:_(s32) = G_ANYEXT %1 (s16)
207+
%18:_(s32) = G_ANYEXT %6 (s16)
208+
%19:_(s32) = G_ANYEXT %11(s16)
209+
%20:_(s32) = G_ANYEXT %16(s16)
210+
%21:_(s32) = G_ADD %17, %18
211+
%22:_(s32) = G_ADD %19, %20
212+
%23:_(s32) = G_ADD %21, %22
213+
$w0 = COPY %23(s32)
214+
RET_ReallyLR implicit $w0
215+
...
216+
---
217+
name: variadic_defs_unmerge_scalar
218+
legalized: true
219+
regBankSelected: false
220+
selected: false
221+
body: |
222+
; CHECK-LABEL: name: variadic_defs_unmerge_scalar
223+
; CHECK: [[COPY:%[0-9]+]]:_(s64) = COPY $d0
224+
; CHECK-NEXT: [[UV0:%[0-9]+]]:_(s16), [[UV1:%[0-9]+]]:_(s16), [[UV2:%[0-9]+]]:_(s16), [[UV3:%[0-9]+]]:_(s16) = G_UNMERGE_VALUES [[COPY]](s64)
225+
; CHECK-NEXT: [[ANYEXT0:%[0-9]+]]:_(s32) = G_ANYEXT [[UV0]](s16)
226+
; CHECK-NEXT: [[ANYEXT1:%[0-9]+]]:_(s32) = G_ANYEXT [[UV1]](s16)
227+
; CHECK-NEXT: [[ANYEXT2:%[0-9]+]]:_(s32) = G_ANYEXT [[UV2]](s16)
228+
; CHECK-NEXT: [[ANYEXT3:%[0-9]+]]:_(s32) = G_ANYEXT [[UV3]](s16)
229+
; CHECK-NEXT: [[ADD0:%[0-9]+]]:_(s32) = G_ADD [[ANYEXT0]], [[ANYEXT1]]
230+
; CHECK-NEXT: [[ADD1:%[0-9]+]]:_(s32) = G_ADD [[ANYEXT2]], [[ANYEXT3]]
231+
; CHECK-NEXT: [[ADD2:%[0-9]+]]:_(s32) = G_ADD [[ADD0]], [[ADD1]]
232+
; CHECK-NEXT: $w0 = COPY [[ADD2]](s32)
233+
; CHECK-NEXT: RET_ReallyLR implicit $w0
234+
bb.0:
235+
%0 :_(s64) = COPY $d0
236+
%1 :_(s16), %2 :_(s16), %3 :_(s16), %4 :_(s16) = G_UNMERGE_VALUES %0(s64)
237+
%5 :_(s16), %6 :_(s16), %7 :_(s16), %8 :_(s16) = G_UNMERGE_VALUES %0(s64)
238+
%9 :_(s16), %10:_(s16), %11:_(s16), %12:_(s16) = G_UNMERGE_VALUES %0(s64)
239+
%13:_(s16), %14:_(s16), %15:_(s16), %16:_(s16) = G_UNMERGE_VALUES %0(s64)
240+
%17:_(s32) = G_ANYEXT %1 (s16)
241+
%18:_(s32) = G_ANYEXT %6 (s16)
242+
%19:_(s32) = G_ANYEXT %11(s16)
243+
%20:_(s32) = G_ANYEXT %16(s16)
244+
%21:_(s32) = G_ADD %17, %18
245+
%22:_(s32) = G_ADD %19, %20
246+
%23:_(s32) = G_ADD %21, %22
247+
$w0 = COPY %23(s32)
248+
RET_ReallyLR implicit $w0
249+
...
250+
---
251+
name: variadic_defs_unmerge_scalar_asym
252+
legalized: true
253+
regBankSelected: false
254+
selected: false
255+
body: |
256+
; CHECK-LABEL: name: variadic_defs_unmerge_scalar_asym
257+
; CHECK: [[COPY:%[0-9]+]]:_(s64) = COPY $d0
258+
; CHECK-NEXT: [[UV0:%[0-9]+]]:_(s16), [[UV1:%[0-9]+]]:_(s16), [[UV2:%[0-9]+]]:_(s16), [[UV3:%[0-9]+]]:_(s16) = G_UNMERGE_VALUES [[COPY]](s64)
259+
; CHECK-NEXT: [[UV01:%[0-9]+]]:_(s32), [[UV23:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[COPY]](s64)
260+
; CHECK-NEXT: [[ANYEXT0:%[0-9]+]]:_(s32) = G_ANYEXT [[UV0]](s16)
261+
; CHECK-NEXT: [[ANYEXT1:%[0-9]+]]:_(s32) = G_ANYEXT [[UV1]](s16)
262+
; CHECK-NEXT: [[ADD0:%[0-9]+]]:_(s32) = G_ADD [[ANYEXT0]], [[ANYEXT1]]
263+
; CHECK-NEXT: [[ADD1:%[0-9]+]]:_(s32) = G_ADD [[UV01]], [[UV23]]
264+
; CHECK-NEXT: [[ADD2:%[0-9]+]]:_(s32) = G_ADD [[ADD0]], [[ADD1]]
265+
; CHECK-NEXT: $w0 = COPY [[ADD2]](s32)
266+
; CHECK-NEXT: RET_ReallyLR implicit $w0
267+
bb.0:
268+
%0 :_(s64) = COPY $d0
269+
%1 :_(s16), %2 :_(s16), %3 :_(s16), %4 :_(s16) = G_UNMERGE_VALUES %0(s64)
270+
%9 :_(s32), %10:_(s32) = G_UNMERGE_VALUES %0(s64)
271+
%5 :_(s16), %6 :_(s16), %7 :_(s16), %8 :_(s16) = G_UNMERGE_VALUES %0(s64)
272+
%11:_(s32), %12:_(s32) = G_UNMERGE_VALUES %0(s64)
273+
%17:_(s32) = G_ANYEXT %1 (s16)
274+
%18:_(s32) = G_ANYEXT %6 (s16)
275+
%21:_(s32) = G_ADD %17, %18
276+
%22:_(s32) = G_ADD %9, %12
277+
%23:_(s32) = G_ADD %21, %22
278+
$w0 = COPY %23(s32)
279+
RET_ReallyLR implicit $w0
280+
...

0 commit comments

Comments
 (0)