PPCAnalyst: Less strict interrupt checks in CanSwapAdjacentOps

[JosJuice]

When these checks were originally added, we didn't have the nice canCauseException attribute. Now we do, so let's use it.

Split out from PR #9857, because it looks like that PR isn't getting reviewed any time soon.

[Rumi-Larry]

Split out from PR #9857, because it looks like that PR isn't getting reviewed any time soon.

I am curious why that tends to happen. Why certain promising pr's just get left abandoned?

[JosJuice]

Because properly reviewing takes effort and skill, especially if the change is to code you're not familiar with. The number of people who are active on Dolphin and understand this part of the codebase is smaller than you might think.

[Rumi-Larry]

@JosJuice I was thinking, wouldn't it be better if there was a unified non-profit organization, that would coordinate and crowdfund emulator projects of all kinds of consoles?

[JosJuice]

Hiring someone to work on an emulator as an actual job costs a lot of money, and doesn't guarantee that you'll get someone who actually knows this stuff better than the volunteers there are right now.

Anyway, this is getting rather off topic. If you want to discuss this further you should join us on IRC.

[Sintendo]

I toyed around with this for a bit last night, mainly to see what cases it allows that it didn't previously. The case that came up most frequently is moving blr downwards further by swapping it with arithmetic instructions. I don't really understand PPCAnalyst well enough to know whether that's a good idea or not.

[JosJuice]

Reordering blr sounds dangerous. When I wrote this patch, I may have been assuming that all branch instructions have canCauseException set to true, but that's not actually the case.

I've added a check to abort if a is a branch. Could you re-test?

What confuses me is how a could end up being blr to begin with... Because of how ReorderInstructionsCore works, a should always be a CR instruction, an instruction that sets the carry flag, or a cmp instruction.

[Sintendo]

It confused me too, but for whatever reason blr is explicitly marked as a CR instruction during analysis: https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/Core/PowerPC/PPCAnalyst.cpp#L649

I'll be sure to give this another go later this week.

[JosJuice]

Good catch!

That's a strange piece of code... My best guess at its purpose is that it tries to make it so that if an instruction writes to a condition register, and then no instruction reads from that condition register before the next blr, we don't need to store the new value of the condition register to ppcState. A kind of optimization based on making assumptions about the ABI. It accomplishes this in a slightly roundabout way, setting outputCR0 = true as a way of making the "Scan for flag dependencies" loop later on set wantsCR0 = false.

Perhaps I will submit a separate PR later to get rid of this. But for now, the check I added should be sufficient for making this PR not break anything.

[JosJuice]

Created it as PR #10284.

[Sintendo]

Gave this another go. blr is indeed no longer considered for swaps and the remaining 'new' swaps all look sane.