-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Implement SET_PC_B64 instruction #2823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some more minor comments from me.
const u32 base_pc = pc_map[setpc_index - 3] + getpc.length; | ||
|
||
const u32 result_pc = static_cast<u32>(static_cast<s32>(base_pc) + signed_offset); | ||
LOG_INFO(Render_Recompiler, "SetPC target: {} + {} = {}", base_pc, signed_offset, result_pc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably remove this log, or at least lower to debug/trace.
AddLabel(pc + inst.length); | ||
} else if (inst.IsConditionalBranch()) { | ||
const u32 true_label = inst.BranchTarget(pc); | ||
const u32 false_label = pc + inst.length; | ||
AddLabel(true_label); | ||
AddLabel(false_label); | ||
} else if (inst.opcode == Opcode::S_ENDPGM) { | ||
const u32 next_label = pc + inst.length; | ||
AddLabel(next_label); | ||
AddLabel(pc + inst.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded change
@@ -18,7 +18,7 @@ bool GcnInst::IsTerminateInstruction() const { | |||
} | |||
|
|||
bool GcnInst::IsUnconditionalBranch() const { | |||
return opcode == Opcode::S_BRANCH; | |||
return opcode == Opcode::S_BRANCH or opcode == Opcode::S_SETPC_B64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer using ||
to or
syntax.
@@ -18,6 +18,8 @@ void Translator::EmitFlowControl(u32 pc, const GcnInst& inst) { | |||
return; | |||
case Opcode::S_GETPC_B64: | |||
return S_GETPC_B64(pc, inst); | |||
case Opcode::S_SETPC_B64: | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need a separate return here, can just let it fallthrough into the existing return;
case below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We process the setpc before here so it will throw an unreachable here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure what you mean, what I’m saying is there’s already a switch case that just returns that you can add this to, there’s no need to put another return here.
I have now implemented the suggestions. |
LGTM, have a few more code style nits but I can probably smooth them out later. I’ll merge in a bit when CI is done. |
* basic impl * minor improvements * clang * more clang * improvements requested by squidbus
* basic impl * minor improvements * clang * more clang * improvements requested by squidbus
* basic impl * minor improvements * clang * more clang * improvements requested by squidbus
Implement this instruction by resolving the target addresses of the setpcs and replacing them with branches.

Fixes unreachable in CUSA02694