This is an archive of the discontinued LLVM Phabricator instance.

Differential D82459

[AMDGPU] Make sure to fix implicit operands on insertBranch
ClosedPublic

Authored by dstuttard on Jun 24 2020, 6:13 AM.

Details

Reviewers
foad
arsenm
Commits
rGe8775c8d817b: [AMDGPU] Make sure to fix implicit operands on insertBranch
Summary

Without fixImplicitOperands we may end up creating default implicit operands
that are the wrong wave size

Includes simple test that provokes insertBranch in the correct way to expose the
issue being fixed.

Change-Id: I92bdcdee9fcb7b4d91529b84e76a48ac8218483e

Diff Detail

Event Timeline

dstuttard created this revision.Jun 24 2020, 6:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2020, 6:13 AM
Herald added subscribers: llvm-commits, kerbowa, hiraditya and 8 others. · View Herald Transcript
dstuttard added reviewers: foad, arsenm.Jun 24 2020, 6:16 AM
arsenm added inline comments.Jun 24 2020, 6:41 AM
llvm/test/CodeGen/AMDGPU/insert-branch-w32.mir
2

wave32 is the default so you don't need the mattr

28

Can you also add a case where the undef needs to be preserved

Harbormaster completed remote builds in B61537: Diff 273008.Jun 24 2020, 6:59 AM
dstuttard updated this revision to Diff 273038.Jun 24 2020, 8:32 AM

Made requested changes

dstuttard marked 2 inline comments as done.Jun 24 2020, 8:33 AM
arsenm added inline comments.Jun 24 2020, 8:42 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
2268

Preexisting complaint, but this function name is bad (and we should really be defining separate wave32 instructions anyway)

arsenm accepted this revision.Jun 24 2020, 8:42 AM
This revision is now accepted and ready to land.Jun 24 2020, 8:42 AM
Closed by commit rGe8775c8d817b: [AMDGPU] Make sure to fix implicit operands on insertBranch (authored by dstuttard). · Explain WhyJun 24 2020, 9:10 AM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B61556: Diff 273038.Jun 24 2020, 10:15 AM

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
4 lines
test/
CodeGen/
AMDGPU/
47 lines

Diff 273062

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

Show First 20 Lines • Show All 2,259 Lines • ▼ Show 20 Linesunsigned SIInstrInfo::insertBranch(MachineBasicBlock &MBB,
if (!FBB) { if (!FBB) {
Cond[1].isUndef(); Cond[1].isUndef();
MachineInstr *CondBr = MachineInstr *CondBr =
BuildMI(&MBB, DL, get(Opcode)) BuildMI(&MBB, DL, get(Opcode))
.addMBB(TBB); .addMBB(TBB);
// Copy the flags onto the implicit condition register operand. // Copy the flags onto the implicit condition register operand.
preserveCondRegFlags(CondBr->getOperand(1), Cond[1]); preserveCondRegFlags(CondBr->getOperand(1), Cond[1]);
fixImplicitOperands(*CondBr);
arsenmUnsubmitted
Not Done
ReplyInline Actions

Preexisting complaint, but this function name is bad (and we should really be defining separate wave32 instructions anyway)

arsenm: Preexisting complaint, but this function name is bad (and we should really be defining separate…
if (BytesAdded) if (BytesAdded)
*BytesAdded = 4; *BytesAdded = 4;
return 1; return 1;
} }
assert(TBB && FBB); assert(TBB && FBB);
▲ Show 20 Lines • Show All 1,045 Lines • ▼ Show 20 Lines
} }
// Set VCC operand with all flags from \p Orig, except for setting it as // Set VCC operand with all flags from \p Orig, except for setting it as
// implicit. // implicit.
static void copyFlagsToImplicitVCC(MachineInstr &MI, static void copyFlagsToImplicitVCC(MachineInstr &MI,
const MachineOperand &Orig) { const MachineOperand &Orig) {
for (MachineOperand &Use : MI.implicit_operands()) { for (MachineOperand &Use : MI.implicit_operands()) {
if (Use.isUse() && Use.getReg() == AMDGPU::VCC) { if (Use.isUse() &&
(Use.getReg() == AMDGPU::VCC || Use.getReg() == AMDGPU::VCC_LO)) {
Use.setIsUndef(Orig.isUndef()); Use.setIsUndef(Orig.isUndef());
Use.setIsKill(Orig.isKill()); Use.setIsKill(Orig.isKill());
return; return;
} }
} }
} }
MachineInstr *SIInstrInfo::buildShrunkInst(MachineInstr &MI, MachineInstr *SIInstrInfo::buildShrunkInst(MachineInstr &MI,
▲ Show 20 Lines • Show All 3,695 Lines • Show Last 20 Lines

llvm/test/CodeGen/AMDGPU/insert-branch-w32.mir

  • This file was added.
# RUN: llc -march=amdgcn -mcpu=gfx1010 -verify-machineinstrs -run-pass branch-folder -o - %s | FileCheck %s
arsenmUnsubmitted
Done
ReplyInline Actions

wave32 is the default so you don't need the mattr

arsenm: wave32 is the default so you don't need the mattr
# Designed to provoke calling SIInstrInfo::insertBranch in wave32 mode
# The implicit $vcc operand should be $vcc_lo in this case
...
# CHECK-LABEL: bb.1:
# CHECK: S_CBRANCH_VCCNZ %bb.1, implicit $vcc_lo
name: _amdgpu_cs_main
body: |
bb.0:
$vgpr1 = V_MOV_B32_e32 1050, implicit $exec
$sgpr0 = S_MOV_B32 1123418112
$vcc_hi = IMPLICIT_DEF
bb.1:
$vgpr0 = COPY killed $vgpr1, implicit $exec
V_CMP_GT_U32_e32 5, $vgpr1, implicit-def $vcc_lo, implicit $exec, implicit-def $vcc
$vcc_lo = S_AND_B32 $exec_lo, $vcc_lo, implicit-def dead $scc
S_CBRANCH_VCCNZ %bb.1, implicit $vcc_lo, implicit $vcc
S_BRANCH %bb.2
bb.2:
$sgpr1 = COPY $sgpr0
S_BRANCH %bb.1
...
arsenmUnsubmitted
Done
ReplyInline Actions

Can you also add a case where the undef needs to be preserved

arsenm: Can you also add a case where the undef needs to be preserved
# CHECK-LABEL: bb.1:
# CHECK: S_CBRANCH_VCCNZ %bb.1, implicit undef $vcc_lo
---
name: _amdgpu_cs_main_undef
body: |
bb.0:
$vgpr1 = V_MOV_B32_e32 1050, implicit $exec
$sgpr0 = S_MOV_B32 1123418112
$vcc_hi = IMPLICIT_DEF
bb.1:
$vgpr0 = COPY killed $vgpr1, implicit $exec
S_CBRANCH_VCCNZ %bb.1, implicit undef $vcc_lo, implicit undef $vcc
S_BRANCH %bb.2
bb.2:
$sgpr1 = COPY $sgpr0
S_BRANCH %bb.1
...