This is an archive of the discontinued LLVM Phabricator instance.

Differential D87674

[AMDGPU] Insert waitcnt after returning from call
ClosedPublic

Authored by Flakebi on Sep 15 2020, 1:15 AM.

Details

Reviewers
arsenm
t-tye
madhur13490
Commits
rGca907bfb57d8: [AMDGPU] Insert waitcnt after returning from call
Summary

When memory operations are outstanding on function calls, either the
caller or the callee can insert a waitcnt to ensure that all reads are
finished.
Calls need some time to be executed, so if the callee inserts the
waitcnt, filling the instruction buffer and waiting for memory will be
interleaved, hiding some latency. This comes at the cost of having a
waitcnt inside functions that may not be needed as no memory operations
are outstanding.

For function calls, this is already implemented. The same principal
applies to returns: If the caller inserts a waitcnt after the call, the
callee does not have to wait and the return and memory operation can be
run in parallel.

This commit implements waiting in the caller after returning from a
function call.

Diff Detail

Event Timeline

Flakebi created this revision.Sep 15 2020, 1:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 1:15 AM
Herald added subscribers: llvm-commits, wenlei, kerbowa and 11 others. · View Herald Transcript
Flakebi requested review of this revision.Sep 15 2020, 1:15 AM
Herald added a subscriber: wdng. · View Herald TranscriptSep 15 2020, 1:15 AM
Harbormaster completed remote builds in B71695: Diff 291821.Sep 15 2020, 1:43 AM
arsenm added inline comments.Sep 15 2020, 7:21 AM
llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
858

In principle this depend on the calling convention, so should retain the argument

1216

Return early and reduce indentation

1226

Why drop the debug loc?

Flakebi updated this revision to Diff 291945.Sep 15 2020, 8:29 AM

Use DebugLoc from call for waitcnt and return early.

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
858

I guess callWaitsOnFunctionEntry should be similar but we do not have an instruction when it is called in runOnMachineFunction.
In the case callWaitsOnFunctionEntry or callWaitsOnFunctionReturn is called for a callee, we can pass the calling convention directly.
Can we get the calling convention somehow from a call instruction?

madhur13490 added inline comments.Sep 15 2020, 8:44 AM
llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
893

please fuse these two "if" conditions. They are already unreadable.

1220

nit: "Don't insert waitcnt if this function returns *immediately*"

arsenm added inline comments.Sep 15 2020, 8:48 AM
llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
858

I think you have to get it from the call target global (which may be null). We would probably need to track this in the call somehow

Flakebi updated this revision to Diff 291952.Sep 15 2020, 9:03 AM

Improve comment as suggested

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
893

Fusing these conditions would change the behavior as the else is executed (and insert waitcnts), so I can’t :)

Harbormaster completed remote builds in B71746: Diff 291945.Sep 15 2020, 9:08 AM
Harbormaster completed remote builds in B71751: Diff 291952.Sep 15 2020, 9:41 AM
madhur13490 added a comment.Sep 15 2020, 10:04 AM
llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
893

Ah! I didn't notice the else part. I think the conditions in first "if" (and also of "else if") can be wrapped inside a function for better readability but I'd leave it to you if you want to clean this up :)

Flakebi updated this revision to Diff 292132.Sep 16 2020, 1:12 AM

Move is gs-done check to own function.

Flakebi added inline comments.Sep 16 2020, 1:14 AM
llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
858

Should I do this as part of this patch or leave it for later, when a calling convention wants other behavior?

Harbormaster completed remote builds in B71835: Diff 292132.Sep 16 2020, 1:45 AM
arsenm added inline comments.Sep 16 2020, 7:28 AM
llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
858

It's a separate change. Should add a fixme that this is a function of the calling convention

madhur13490 accepted this revision.Sep 16 2020, 7:29 AM
This revision is now accepted and ready to land.Sep 16 2020, 7:29 AM
Flakebi updated this revision to Diff 292216.Sep 16 2020, 7:39 AM

Add fixme that wait on function entry/return should depend on calling convention.

Harbormaster completed remote builds in B71876: Diff 292216.Sep 16 2020, 8:12 AM
Closed by commit rGca907bfb57d8: [AMDGPU] Insert waitcnt after returning from call (authored by sebastian-ne). · Explain WhySep 23 2020, 3:18 AM
This revision was automatically updated to reflect the committed changes.
sebastian-ne added a commit: rGca907bfb57d8: [AMDGPU] Insert waitcnt after returning from call.
Herald added a subscriber: arichardson. · View Herald TranscriptSep 23 2020, 3:18 AM
michel.daenzer added a subscriber: michel.daenzer.Sep 23 2020, 7:54 AM

This completely broke the Mesa radeonsi driver on Navi 14. Xorg + xterm come up with major corruption & psychedelic colours.

sebastian-ne added a reverting change: rGa343b9b03277: Revert "[AMDGPU] Insert waitcnt after returning from call".Sep 23 2020, 8:17 AM
Flakebi added a comment.Sep 23 2020, 8:19 AM

Thanks for the heads-up, I reverted it for now.

mareko added a subscriber: mareko.Sep 23 2020, 9:40 AM

Yes, this commit is incorrect. It completely breaks code linking in Mesa OpenGL. s_waitcnt is required at the end of all global functions that return values.

Please revert. @nhaehnle

madhur13490 added a comment.Sep 23 2020, 9:49 AM
In D87674#2290396, @mareko wrote:

Yes, this commit is incorrect. It completely breaks code linking in Mesa OpenGL. s_waitcnt is required at the end of all global functions that return values.

Please revert. @nhaehnle

I don't understand why would it fail. This patch just moves s_waitcnt to the caller so they would be executed anyway. I think I am missing something. It would be helpful to root cause if we can isolate to a small test case.

arsenm added a comment.Sep 23 2020, 9:50 AM
In D87674#2290418, @madhur13490 wrote:
In D87674#2290396, @mareko wrote:

Yes, this commit is incorrect. It completely breaks code linking in Mesa OpenGL. s_waitcnt is required at the end of all global functions that return values.

Please revert. @nhaehnle

I don't understand why would it fail. This patch just moves s_waitcnt to the caller so they would be executed anyway. I think I am missing something. It would be helpful to root cause if we can isolate to a small test case.

Shader returns aren't real returns and the "caller" doesn't wait

madhur13490 added a comment.Sep 23 2020, 9:53 AM
In D87674#2290419, @arsenm wrote:
In D87674#2290418, @madhur13490 wrote:
In D87674#2290396, @mareko wrote:

Yes, this commit is incorrect. It completely breaks code linking in Mesa OpenGL. s_waitcnt is required at the end of all global functions that return values.

Please revert. @nhaehnle

I don't understand why would it fail. This patch just moves s_waitcnt to the caller so they would be executed anyway. I think I am missing something. It would be helpful to root cause if we can isolate to a small test case.

Shader returns aren't real returns and the "caller" doesn't wait

I see. So how should this be implemented? May be we conditionalize this patch just for compute?

mareko added a comment.EditedSep 23 2020, 9:57 AM
In D87674#2290419, @arsenm wrote:
In D87674#2290418, @madhur13490 wrote:
In D87674#2290396, @mareko wrote:

Yes, this commit is incorrect. It completely breaks code linking in Mesa OpenGL. s_waitcnt is required at the end of all global functions that return values.

Please revert. @nhaehnle

I don't understand why would it fail. This patch just moves s_waitcnt to the caller so they would be executed anyway. I think I am missing something. It would be helpful to root cause if we can isolate to a small test case.

Shader returns aren't real returns and the "caller" doesn't wait

That's right. There are no callers. Functions that return SGPRs and VGPRs don't jump anywhere. Instead, return means that another function will be appended at the shader binary level after the last instruction, consuming those SGPRs and VGPRs. The other function doesn't know whether it was appended at the end of another shader binary, or whether it's the real beginning of the shader and the SGPRs and VGPRs were initialized by the register init machines.

nhaehnle added a comment.Sep 23 2020, 10:02 AM
In D87674#2290434, @madhur13490 wrote:
In D87674#2290419, @arsenm wrote:
In D87674#2290418, @madhur13490 wrote:
In D87674#2290396, @mareko wrote:

Yes, this commit is incorrect. It completely breaks code linking in Mesa OpenGL. s_waitcnt is required at the end of all global functions that return values.

Please revert. @nhaehnle

I don't understand why would it fail. This patch just moves s_waitcnt to the caller so they would be executed anyway. I think I am missing something. It would be helpful to root cause if we can isolate to a small test case.

Shader returns aren't real returns and the "caller" doesn't wait

I see. So how should this be implemented? May be we conditionalize this patch just for compute?

How waitcnt are handled is a function of the calling convention, so that's what should be tested for here. Calls to and returns from functions with a default calling convention can be handled in the new way, while other cases need to be left unchanged for now. The mistake was to believe that this could be delayed for later as a "fixme".

arsenm added a comment.Sep 23 2020, 10:04 AM

This could also just check the specific return opcodes (we could maybe even remove the isReturn from the shader pseudo-return)

madhur13490 added a comment.Sep 23 2020, 10:13 AM

The revert also highlights the problem that we don't have representative test in llvm/test :(

nhaehnle added a comment.Sep 24 2020, 12:22 AM
In D87674#2290548, @madhur13490 wrote:

The revert also highlights the problem that we don't have representative test in llvm/test :(

Don't we? Isn't the problem rather that a lit test alone isn't really authoritative on whether the generated code is correct? There are cases where it's easy to convince yourself that a change in codegen is correct even though it isn't, if you're only staring at the assembly.

michel.daenzer added a comment.Sep 24 2020, 1:20 AM
In D87674#2290548, @madhur13490 wrote:

The revert also highlights the problem that we don't have representative test in llvm/test :(

To me, it rather highlights that there still doesn't seem to be even smoke-testing with Mesa before merging LLVM AMDGPU backend changes, let alone automated regression testing.

mareko added a comment.Sep 24 2020, 2:58 AM

Using the new return behavior only for the calling conventions that Mesa doesn't use sounds like the right solution.

Large Diff

This large diff affects 120 files. Files without inline comments have been collapsed. Expand All Files

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
78 lines
test/
CodeGen/
AMDGPU/
GlobalISel/
2 lines
5 lines
6 lines
3 lines
4 lines
8 lines
2 lines
2 lines
2 lines
12 lines
2 lines
4 lines
14 lines
14 lines
14 lines
4 lines
5 lines
84 lines
126 lines
28 lines
34 lines
24 lines
16 lines
24 lines
31 lines
30 lines
6 lines
6 lines
6 lines
6 lines
6 lines
19 lines
24 lines
12 lines
22 lines
8 lines
8 lines
6 lines
1 line
42 lines
2 lines
15 lines
8 lines
5 lines
2 lines
6 lines
2 lines
17 lines
16 lines
8 lines
10 lines
30 lines
3 lines
10 lines
2 lines
43 lines
2 lines
8 lines
96 lines
2 lines
27 lines
2 lines
1 line
2 lines
6 lines
11 lines
21 lines
7 lines
2 lines
54 lines
54 lines
215 lines
28 lines
2 lines
94 lines
20 lines
208 lines
19 lines
19 lines
1 line
4 lines
7 lines
18 lines
3 lines
11 lines
11 lines
3 lines
2 lines
39 lines
136 lines
12 lines
12 lines
1 line
1 line
1 line
3 lines
7 lines
1 line
3 lines
2 lines
50 lines
50 lines
2 lines
1 line
2 lines
2 lines
2 lines
2 lines
1 line
31 lines
6 lines
1 line
24 lines
3 lines
1 line
14 lines
1 line
tools/
UpdateTestChecks/
update_llc_test_checks/
Inputs/
1 line
1 line

Diff 293684

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

Show First 20 Lines • Show All 456 Lines • ▼ Show 20 Lines#ifndef NDEBUG
} }
#endif // NDEBUG #endif // NDEBUG
} }
bool mayAccessLDSThroughFlat(const MachineInstr &MI) const; bool mayAccessLDSThroughFlat(const MachineInstr &MI) const;
bool generateWaitcntInstBefore(MachineInstr &MI, bool generateWaitcntInstBefore(MachineInstr &MI,
WaitcntBrackets &ScoreBrackets, WaitcntBrackets &ScoreBrackets,
MachineInstr *OldWaitcntInstr); MachineInstr *OldWaitcntInstr);
bool generateWaitcntInstAfter(MachineInstr &MI,
WaitcntBrackets &ScoreBrackets,
MachineInstr *OldWaitcntInstr);
void updateEventWaitcntAfter(MachineInstr &Inst, void updateEventWaitcntAfter(MachineInstr &Inst,
WaitcntBrackets *ScoreBrackets); WaitcntBrackets *ScoreBrackets);
bool insertWaitcntInBlock(MachineFunction &MF, MachineBasicBlock &Block, bool insertWaitcntInBlock(MachineFunction &MF, MachineBasicBlock &Block,
WaitcntBrackets &ScoreBrackets); WaitcntBrackets &ScoreBrackets);
}; };
} // end anonymous namespace } // end anonymous namespace
▲ Show 20 Lines • Show All 346 Lines • ▼ Show 20 Lines
} }
static bool readsVCCZ(const MachineInstr &MI) { static bool readsVCCZ(const MachineInstr &MI) {
unsigned Opc = MI.getOpcode(); unsigned Opc = MI.getOpcode();
return (Opc == AMDGPU::S_CBRANCH_VCCNZ || Opc == AMDGPU::S_CBRANCH_VCCZ) && return (Opc == AMDGPU::S_CBRANCH_VCCNZ || Opc == AMDGPU::S_CBRANCH_VCCZ) &&
!MI.getOperand(1).isUndef(); !MI.getOperand(1).isUndef();
} }
// For jumps like function calls and returns, we insert waitcnts at the jump
// destination. The jump adds latency because new instructions need to be
// fetched, waiting for outstanding memory operations after the jump means
// that the memory latency can be overlapped with the jump latency.
// FIXME callWaitsOnFunctionEntry and callWaitsOnFunctionReturn should depend
// on the calling convention. We need to track the calling convention for
// call instructions, so it is available here.
/// \returns true if the callee inserts an s_waitcnt 0 on function entry. /// \returns true if the callee inserts an s_waitcnt 0 on function entry.
static bool callWaitsOnFunctionEntry(const MachineInstr &MI) { static bool callWaitsOnFunctionEntry() {
// Currently all conventions wait, but this may not always be the case. // Currently all conventions wait, but this may not always be the case.
// //
// TODO: If IPRA is enabled, and the callee is isSafeForNoCSROpt, it may make // TODO: If IPRA is enabled, and the callee is isSafeForNoCSROpt, it may make
// senses to omit the wait and do it in the caller. // senses to omit the wait and do it in the caller.
return true; return true;
} }
/// \returns true if the callee is expected to wait for any outstanding waits /// \returns true if the callee is expected to wait for any outstanding waits
/// before returning. /// before returning.
static bool callWaitsOnFunctionReturn(const MachineInstr &MI) { static bool callWaitsOnFunctionReturn(const MachineInstr &MI) { return false; }
return true;
/// \returns true if the instruction is an s_sendmsg of gs-done.
static bool isSendGsDoneMessage(const MachineInstr &MI) {
return (MI.getOpcode() == AMDGPU::S_SENDMSG ||
MI.getOpcode() == AMDGPU::S_SENDMSGHALT) &&
(MI.getOperand(0).getImm() & AMDGPU::SendMsg::ID_MASK_) ==
AMDGPU::SendMsg::ID_GS_DONE;
} }
arsenmUnsubmitted
Not Done
ReplyInline Actions

In principle this depend on the calling convention, so should retain the argument

arsenm: In principle this depend on the calling convention, so should retain the argument
FlakebiAuthorUnsubmitted
Not Done
ReplyInline Actions

I guess callWaitsOnFunctionEntry should be similar but we do not have an instruction when it is called in runOnMachineFunction.
In the case callWaitsOnFunctionEntry or callWaitsOnFunctionReturn is called for a callee, we can pass the calling convention directly.
Can we get the calling convention somehow from a call instruction?

Flakebi: I guess `callWaitsOnFunctionEntry` should be similar but we do not have an instruction when it…
arsenmUnsubmitted
Not Done
ReplyInline Actions

I think you have to get it from the call target global (which may be null). We would probably need to track this in the call somehow

arsenm: I think you have to get it from the call target global (which may be null). We would probably…
FlakebiAuthorUnsubmitted
Not Done
ReplyInline Actions

Should I do this as part of this patch or leave it for later, when a calling convention wants other behavior?

Flakebi: Should I do this as part of this patch or leave it for later, when a calling convention wants…
arsenmUnsubmitted
Not Done
ReplyInline Actions

It's a separate change. Should add a fixme that this is a function of the calling convention

arsenm: It's a separate change. Should add a fixme that this is a function of the calling convention
/// Generate s_waitcnt instruction to be placed before cur_Inst. /// Generate s_waitcnt instruction to be placed before cur_Inst.
/// Instructions of a given type are returned in order, /// Instructions of a given type are returned in order,
/// but instructions of different types can complete out of order. /// but instructions of different types can complete out of order.
/// We rely on this in-order completion /// We rely on this in-order completion
/// and simply assign a score to the memory access instructions. /// and simply assign a score to the memory access instructions.
/// We keep track of the active "score bracket" to determine /// We keep track of the active "score bracket" to determine
/// if an access of a memory read requires an s_waitcnt /// if an access of a memory read requires an s_waitcnt
Show All 16 Linesbool SIInsertWaitcnts::generateWaitcntInstBefore(
if (MI.getOpcode() == AMDGPU::BUFFER_WBINVL1 || if (MI.getOpcode() == AMDGPU::BUFFER_WBINVL1 ||
MI.getOpcode() == AMDGPU::BUFFER_WBINVL1_SC || MI.getOpcode() == AMDGPU::BUFFER_WBINVL1_SC ||
MI.getOpcode() == AMDGPU::BUFFER_WBINVL1_VOL || MI.getOpcode() == AMDGPU::BUFFER_WBINVL1_VOL ||
MI.getOpcode() == AMDGPU::BUFFER_GL0_INV || MI.getOpcode() == AMDGPU::BUFFER_GL0_INV ||
MI.getOpcode() == AMDGPU::BUFFER_GL1_INV) { MI.getOpcode() == AMDGPU::BUFFER_GL1_INV) {
Wait.VmCnt = 0; Wait.VmCnt = 0;
} }
if (MI.getOpcode() == AMDGPU::SI_RETURN_TO_EPILOG ||
MI.getOpcode() == AMDGPU::S_SETPC_B64_return ||
(MI.isReturn() && MI.isCall() && !callWaitsOnFunctionEntry())) {
madhur13490Unsubmitted
Not Done
ReplyInline Actions

please fuse these two "if" conditions. They are already unreadable.

madhur13490: please fuse these two "if" conditions. They are already unreadable.
FlakebiAuthorUnsubmitted
Done
ReplyInline Actions

Fusing these conditions would change the behavior as the else is executed (and insert waitcnts), so I can’t :)

Flakebi: Fusing these conditions would change the behavior as the else is executed (and insert waitcnts)…
madhur13490Unsubmitted
Not Done
ReplyInline Actions

Ah! I didn't notice the else part. I think the conditions in first "if" (and also of "else if") can be wrapped inside a function for better readability but I'd leave it to you if you want to clean this up :)

madhur13490: Ah! I didn't notice the else part. I think the conditions in first "if" (and also of "else if")…
// All waits must be resolved at call return. // All waits must be resolved at call return.
// NOTE: this could be improved with knowledge of all call sites or // NOTE: this could be improved with knowledge of all call sites or
// with knowledge of the called routines. // with knowledge of the called routines.
if (MI.getOpcode() == AMDGPU::SI_RETURN_TO_EPILOG || if (callWaitsOnFunctionReturn(MI))
MI.getOpcode() == AMDGPU::S_SETPC_B64_return ||
(MI.isReturn() && MI.isCall() && !callWaitsOnFunctionEntry(MI))) {
Wait = Wait.combined(AMDGPU::Waitcnt::allZero(ST->hasVscnt())); Wait = Wait.combined(AMDGPU::Waitcnt::allZero(ST->hasVscnt()));
} } else if (isSendGsDoneMessage(MI)) {
// Resolve vm waits before gs-done. // Resolve vm waits before gs-done.
else if ((MI.getOpcode() == AMDGPU::S_SENDMSG ||
MI.getOpcode() == AMDGPU::S_SENDMSGHALT) &&
((MI.getOperand(0).getImm() & AMDGPU::SendMsg::ID_MASK_) ==
AMDGPU::SendMsg::ID_GS_DONE)) {
Wait.VmCnt = 0; Wait.VmCnt = 0;
} }
#if 0 // TODO: the following blocks of logic when we have fence. #if 0 // TODO: the following blocks of logic when we have fence.
else if (MI.getOpcode() == SC_FENCE) { else if (MI.getOpcode() == SC_FENCE) {
const unsigned int group_size = const unsigned int group_size =
context->shader_info->GetMaxThreadGroupSize(); context->shader_info->GetMaxThreadGroupSize();
// group_size == 0 means thread group size is unknown at compile time // group_size == 0 means thread group size is unknown at compile time
const bool group_is_multi_wave = const bool group_is_multi_wave =
▲ Show 20 Lines • Show All 56 Lines • ▼ Show 20 Lines if (MI.modifiesRegister(AMDGPU::EXEC, TRI)) {
if (ScoreBrackets.hasPendingEvent(EXP_GPR_LOCK) || if (ScoreBrackets.hasPendingEvent(EXP_GPR_LOCK) ||
ScoreBrackets.hasPendingEvent(EXP_PARAM_ACCESS) || ScoreBrackets.hasPendingEvent(EXP_PARAM_ACCESS) ||
ScoreBrackets.hasPendingEvent(EXP_POS_ACCESS) || ScoreBrackets.hasPendingEvent(EXP_POS_ACCESS) ||
ScoreBrackets.hasPendingEvent(GDS_GPR_LOCK)) { ScoreBrackets.hasPendingEvent(GDS_GPR_LOCK)) {
Wait.ExpCnt = 0; Wait.ExpCnt = 0;
} }
} }
if (MI.isCall() && callWaitsOnFunctionEntry(MI)) { if (MI.isCall() && callWaitsOnFunctionEntry()) {
// The function is going to insert a wait on everything in its prolog. // The function is going to insert a wait on everything in its prolog.
// This still needs to be careful if the call target is a load (e.g. a GOT // This still needs to be careful if the call target is a load (e.g. a GOT
// load). We also need to check WAW depenancy with saved PC. // load). We also need to check WAW depenancy with saved PC.
Wait = AMDGPU::Waitcnt(); Wait = AMDGPU::Waitcnt();
int CallAddrOpIdx = int CallAddrOpIdx =
AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::src0); AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::src0);
▲ Show 20 Lines • Show All 222 Lines • ▼ Show 20 Lines if (Wait.VsCnt != ~0u) {
LLVM_DEBUG(dbgs() << "generateWaitcntInstBefore\n" LLVM_DEBUG(dbgs() << "generateWaitcntInstBefore\n"
<< "Old Instr: " << MI << "Old Instr: " << MI
<< "New Instr: " << *SWaitInst << '\n'); << "New Instr: " << *SWaitInst << '\n');
} }
return Modified; return Modified;
} }
bool SIInsertWaitcnts::generateWaitcntInstAfter(MachineInstr &MI,
WaitcntBrackets &ScoreBrackets,
MachineInstr *OldWaitcntInstr) {
// Insert waitcnts after function calls (that are not tail calls)
if (!MI.isCall() || MI.isTerminator() || callWaitsOnFunctionReturn(MI))
arsenmUnsubmitted
Not Done
ReplyInline Actions

Return early and reduce indentation

arsenm: Return early and reduce indentation
return false;
auto I = ++MI.getIterator();
// Don't insert waitcnt if this function returns immediately
madhur13490Unsubmitted
Not Done
ReplyInline Actions

nit: "Don't insert waitcnt if this function returns *immediately*"

madhur13490: nit: "Don't insert waitcnt if this function returns *immediately*"
if (I.isEnd() || I->isReturn())
return false;
DebugLoc DL = MI.getDebugLoc();
BuildMI(*MI.getParent(), I, DL, TII->get(AMDGPU::S_WAITCNT)).addImm(0);
if (ST->hasVscnt())
arsenmUnsubmitted
Not Done
ReplyInline Actions

Why drop the debug loc?

arsenm: Why drop the debug loc?
BuildMI(*MI.getParent(), I, DL, TII->get(AMDGPU::S_WAITCNT_VSCNT))
.addReg(AMDGPU::SGPR_NULL, RegState::Undef)
.addImm(0);
return true;
}
// This is a flat memory operation. Check to see if it has memory // This is a flat memory operation. Check to see if it has memory
// tokens for both LDS and Memory, and if so mark it as a flat. // tokens for both LDS and Memory, and if so mark it as a flat.
bool SIInsertWaitcnts::mayAccessLDSThroughFlat(const MachineInstr &MI) const { bool SIInsertWaitcnts::mayAccessLDSThroughFlat(const MachineInstr &MI) const {
if (MI.memoperands_empty()) if (MI.memoperands_empty())
return true; return true;
for (const MachineMemOperand *Memop : MI.memoperands()) { for (const MachineMemOperand *Memop : MI.memoperands()) {
unsigned AS = Memop->getAddrSpace(); unsigned AS = Memop->getAddrSpace();
▲ Show 20 Lines • Show All 59 Lines • ▼ Show 20 Lines if (TII->isDS(Inst) && TII->usesLGKM_CNT(Inst)) {
if (ST->vmemWriteNeedsExpWaitcnt() && if (ST->vmemWriteNeedsExpWaitcnt() &&
(Inst.mayStore() || AMDGPU::getAtomicNoRetOp(Inst.getOpcode()) != -1)) { (Inst.mayStore() || AMDGPU::getAtomicNoRetOp(Inst.getOpcode()) != -1)) {
ScoreBrackets->updateByEvent(TII, TRI, MRI, VMW_GPR_LOCK, Inst); ScoreBrackets->updateByEvent(TII, TRI, MRI, VMW_GPR_LOCK, Inst);
} }
} else if (TII->isSMRD(Inst)) { } else if (TII->isSMRD(Inst)) {
ScoreBrackets->updateByEvent(TII, TRI, MRI, SMEM_ACCESS, Inst); ScoreBrackets->updateByEvent(TII, TRI, MRI, SMEM_ACCESS, Inst);
} else if (Inst.isCall()) { } else if (Inst.isCall()) {
if (callWaitsOnFunctionReturn(Inst)) { // Act as a wait on everything. Either the callee waits or we insert a wait.
// Act as a wait on everything
ScoreBrackets->applyWaitcnt(AMDGPU::Waitcnt::allZero(ST->hasVscnt())); ScoreBrackets->applyWaitcnt(AMDGPU::Waitcnt::allZero(ST->hasVscnt()));
} else { } else {
// May need to way wait for anything.
ScoreBrackets->applyWaitcnt(AMDGPU::Waitcnt());
}
} else {
switch (Inst.getOpcode()) { switch (Inst.getOpcode()) {
case AMDGPU::S_SENDMSG: case AMDGPU::S_SENDMSG:
case AMDGPU::S_SENDMSGHALT: case AMDGPU::S_SENDMSGHALT:
ScoreBrackets->updateByEvent(TII, TRI, MRI, SQ_MESSAGE, Inst); ScoreBrackets->updateByEvent(TII, TRI, MRI, SQ_MESSAGE, Inst);
break; break;
case AMDGPU::EXP: case AMDGPU::EXP:
case AMDGPU::EXP_DONE: { case AMDGPU::EXP_DONE: {
int Imm = TII->getNamedOperand(Inst, AMDGPU::OpName::tgt)->getImm(); int Imm = TII->getNamedOperand(Inst, AMDGPU::OpName::tgt)->getImm();
▲ Show 20 Lines • Show All 159 Lines • ▼ Show 20 Lines if (!ST->partialVCCWritesUpdateVCCZ()) {
VCCZCorrect = false; VCCZCorrect = false;
else if (Inst.definesRegister(AMDGPU::VCC)) else if (Inst.definesRegister(AMDGPU::VCC))
VCCZCorrect = true; VCCZCorrect = true;
} }
// Generate an s_waitcnt instruction to be placed before // Generate an s_waitcnt instruction to be placed before
// cur_Inst, if needed. // cur_Inst, if needed.
Modified |= generateWaitcntInstBefore(Inst, ScoreBrackets, OldWaitcntInstr); Modified |= generateWaitcntInstBefore(Inst, ScoreBrackets, OldWaitcntInstr);
Modified |= generateWaitcntInstAfter(Inst, ScoreBrackets, OldWaitcntInstr);
OldWaitcntInstr = nullptr; OldWaitcntInstr = nullptr;
updateEventWaitcntAfter(Inst, &ScoreBrackets); updateEventWaitcntAfter(Inst, &ScoreBrackets);
#if 0 // TODO: implement resource type check controlled by options with ub = LB. #if 0 // TODO: implement resource type check controlled by options with ub = LB.
// If this instruction generates a S_SETVSKIP because it is an // If this instruction generates a S_SETVSKIP because it is an
// indexed resource, and we are on Tahiti, then it will also force // indexed resource, and we are on Tahiti, then it will also force
// an S_WAITCNT vmcnt(0) // an S_WAITCNT vmcnt(0)
▲ Show 20 Lines • Show All 161 Lines • ▼ Show 20 Lines for (MachineBasicBlock *MBB : EndPgmBlocks) {
!SeenDCacheWB) { !SeenDCacheWB) {
Modified = true; Modified = true;
BuildMI(*MBB, I, I->getDebugLoc(), TII->get(AMDGPU::S_DCACHE_WB)); BuildMI(*MBB, I, I->getDebugLoc(), TII->get(AMDGPU::S_DCACHE_WB));
} }
} }
} }
} }
if (!MFI->isEntryFunction()) { if (!MFI->isEntryFunction() && callWaitsOnFunctionEntry()) {
// Wait for any outstanding memory operations that the input registers may // Wait for any outstanding memory operations that the input registers may
// depend on. We can't track them and it's better to the wait after the // depend on. We can't track them and it's better to the wait after the
// costly call sequence. // costly call sequence.
// TODO: Could insert earlier and schedule more liberally with operations // TODO: Could insert earlier and schedule more liberally with operations
// that only use caller preserved registers. // that only use caller preserved registers.
MachineBasicBlock &EntryBB = MF.front(); MachineBasicBlock &EntryBB = MF.front();
MachineBasicBlock::iterator I = EntryBB.begin(); MachineBasicBlock::iterator I = EntryBB.begin();
Show All 14 Lines

llvm/test/CodeGen/AMDGPU/GlobalISel/cvt_f32_ubyte.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/divergent-control-flow.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/dynamic-alloca-uniform.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement-stack-lower.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement.i128.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement.i16.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/fmax_legacy.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/fmin_legacy.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/image_ls_mipmap_zero.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/insertelement.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/lds-global-non-entry-func.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.ds.fadd.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.ds.fmax.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.ds.fmin.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.global.atomic.csub.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.global.atomic.fadd.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.atomic.dim.a16.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.atomic.dim.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.gather4.a16.dim.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.gather4.dim.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.gather4.o.dim.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.getresinfo.a16.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.getresinfo.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.load.1d.d16.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.load.1d.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.load.2d.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.load.2darraymsaa.a16.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.load.2darraymsaa.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.load.3d.a16.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.load.3d.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.sample.g16.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.sample.ltolz.a16.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.sample.ltolz.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/load-constant.96.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/load-local.128.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/load-local.96.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/load-unaligned.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/localizer.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/mubuf-global.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/non-entry-alloca.ll

llvm/test/CodeGen/AMDGPU/GlobalISel/zextload.ll

llvm/test/CodeGen/AMDGPU/atomic_load_local.ll

llvm/test/CodeGen/AMDGPU/atomic_store_local.ll

llvm/test/CodeGen/AMDGPU/call-argument-types.ll

llvm/test/CodeGen/AMDGPU/call-preserved-registers.ll

llvm/test/CodeGen/AMDGPU/call-waitcnt.ll

llvm/test/CodeGen/AMDGPU/callee-frame-setup.ll

llvm/test/CodeGen/AMDGPU/callee-special-input-vgprs.ll

llvm/test/CodeGen/AMDGPU/chain-hi-to-lo.ll

llvm/test/CodeGen/AMDGPU/cross-block-use-is-not-abi-copy.ll

llvm/test/CodeGen/AMDGPU/cvt_f32_ubyte.ll

llvm/test/CodeGen/AMDGPU/fast-unaligned-load-store.global.ll

llvm/test/CodeGen/AMDGPU/fast-unaligned-load-store.private.ll

llvm/test/CodeGen/AMDGPU/function-args.ll

llvm/test/CodeGen/AMDGPU/function-returns.ll

llvm/test/CodeGen/AMDGPU/global-saddr-atomics.gfx1030.ll

llvm/test/CodeGen/AMDGPU/global-saddr-atomics.ll

llvm/test/CodeGen/AMDGPU/global-saddr-load.ll

llvm/test/CodeGen/AMDGPU/hsa-func.ll

llvm/test/CodeGen/AMDGPU/imm16.ll

llvm/test/CodeGen/AMDGPU/infer-uniform-load-shader.ll

llvm/test/CodeGen/AMDGPU/insert_vector_elt.ll

llvm/test/CodeGen/AMDGPU/lds-global-non-entry-func.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.buffer.load.dwordx3.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.buffer.load.format.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.buffer.load.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.ds.ordered.add.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.ds.ordered.swap.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.a16.dim.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.a16.encode.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.dim.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.gather4.a16.dim.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.getlod.dim.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.a16.dim.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.d16.dim.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.dim.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.g16.encode.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.g16.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.implicit.buffer.ptr.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.implicitarg.ptr.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.raw.buffer.load.format.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.raw.buffer.load.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.raw.tbuffer.load.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.struct.buffer.load.format.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.struct.buffer.load.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.struct.tbuffer.load.ll

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.tbuffer.load.ll

llvm/test/CodeGen/AMDGPU/load-hi16.ll

llvm/test/CodeGen/AMDGPU/load-lo16.ll

llvm/test/CodeGen/AMDGPU/load-local.128.ll

llvm/test/CodeGen/AMDGPU/load-local.96.ll

llvm/test/CodeGen/AMDGPU/local-stack-alloc-block-sp-reference.ll

llvm/test/CodeGen/AMDGPU/mad-mix-hi.ll

llvm/test/CodeGen/AMDGPU/memcpy-fixed-align.ll

llvm/test/CodeGen/AMDGPU/memory_clause.ll

llvm/test/CodeGen/AMDGPU/mul24-pass-ordering.ll

llvm/test/CodeGen/AMDGPU/multi-divergent-exit-region.ll

llvm/test/CodeGen/AMDGPU/nested-calls.ll

llvm/test/CodeGen/AMDGPU/non-entry-alloca.ll

llvm/test/CodeGen/AMDGPU/offset-split-flat.ll

llvm/test/CodeGen/AMDGPU/offset-split-global.ll

llvm/test/CodeGen/AMDGPU/ret_jump.ll

llvm/test/CodeGen/AMDGPU/scalar-store-cache-flush.mir

llvm/test/CodeGen/AMDGPU/shl.ll

llvm/test/CodeGen/AMDGPU/sibling-call.ll

llvm/test/CodeGen/AMDGPU/smrd.ll

llvm/test/CodeGen/AMDGPU/stack-pointer-offset-relative-frameindex.ll

llvm/test/CodeGen/AMDGPU/stack-realign.ll

llvm/test/CodeGen/AMDGPU/store-hi16.ll

llvm/test/CodeGen/AMDGPU/store-weird-sizes.ll

llvm/test/CodeGen/AMDGPU/undefined-subreg-liverange.ll

llvm/test/CodeGen/AMDGPU/vector_shuffle.packed.ll

llvm/test/CodeGen/AMDGPU/vgpr-tuple-allocation.ll

llvm/test/CodeGen/AMDGPU/visit-physreg-vgpr-imm-folding-bug.ll

llvm/test/CodeGen/AMDGPU/waitcnt-vscnt.ll

llvm/test/CodeGen/AMDGPU/wave32.ll

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/amdgpu_generated_funcs.ll.generated.expected

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/amdgpu_generated_funcs.ll.nogenerated.expected