XEmitter: Add enum class Force5Bytes #11834
Conversation
|
I'd prefer if you used an enum class to avoid invalid usage, instead. |
Dentomologist
force-pushed
the
xemitter_use_pseudonamed_parameter_force5bytes
branch
from
e8eb05d
to
4bef57c
Compare
Dentomologist
changed the title
| if (test_bit & 8) | ||
| pDontBranch = J_CC(condition ? CC_GE : CC_L, true); // Test < 0, so jump over if >= 0. | ||
| pDontBranch = | ||
| J_CC(condition ? CC_GE : CC_L, Force5Bytes::Yes); // Test < 0, so jump over if >= 0. | ||
| else if (test_bit & 4) | ||
| pDontBranch = J_CC(condition ? CC_LE : CC_G, true); // Test > 0, so jump over if <= 0. | ||
| pDontBranch = | ||
| J_CC(condition ? CC_LE : CC_G, Force5Bytes::Yes); // Test > 0, so jump over if <= 0. | ||
| else if (test_bit & 2) | ||
| pDontBranch = J_CC(condition ? CC_NE : CC_E, true); // Test = 0, so jump over if != 0. | ||
| pDontBranch = | ||
| J_CC(condition ? CC_NE : CC_E, Force5Bytes::Yes); // Test = 0, so jump over if != 0. | ||
| else // SO bit, do not branch (we don't emulate SO for cmp). | ||
| pDontBranch = J(true); | ||
| pDontBranch = J(Force5Bytes::Yes); |
There was a problem hiding this comment.
These need braces now. Probably a move the comments above the statement, too.
|
Looks good otherwise. |
|
(One could argue whether |
Dentomologist
force-pushed
the
xemitter_use_pseudonamed_parameter_force5bytes
branch
from
4bef57c
to
a3e3651
Compare
|
now what did you do here? |
Dentomologist
force-pushed
the
xemitter_use_pseudonamed_parameter_force5bytes
branch
from
a3e3651
to
ee108b3
Compare
|
I accidently included a commit with some unrelated changes. What I actually did was notice these functions each mainly have two separate control flows, and should probably just be separate functions instead. So J is now J_Short and J_Near, etc. The naming convention of similar variant functions in xemitter is inconsistent, so I picked something that seemed reasonable but let me know if it should be something else. Notes:
|
|
I'm not sure whether this is really better, tbqh. @JosJuice, opinions? Also, why |
|
Some googling gave me the impression those were the names for those types of jumps (eg https://www.felixcloutier.com/x86/jmp.html), but if that's wrong or I've misunderstood it I can change the names to whatever. As for the approach itself, I think you can find clearer explanations here or here than I would come up with off the top of my head. Granted that the Force5Bytes enum is clearer than a literal bool, and if we decide not to do the current approach I'll revert back to it, but it's still acting as an effective boolean that conflates two different actions into one. |
I'm not a huge fan. From the perspective of the person writing x86 assembly, the two are the same thing. Even though they're implemented differently, from the perspective of what they actually do, they're identical. That's just my opinion, though. |
|
Is it not the case that people writing x86 in Dolphin's JIT already do have to treat Short/Near differently, in order to know whether to pass true/Force5Bytes::Yes or not? If it is then splitting the functions like this just makes it explicit to the caller what those parameters would be doing, while getting rid of the run-type overhead of handling the parameter. |
|
Yes, that is true, but doing it this way just feels weird to me somehow as an assembly programmer. I don't have a better argument than that. |
|
I'll revert back to the enum class. What name should I use: Force5Bytes::Yes/No, OffsetBytes::Four/One, Jump::Near/Short, or something else? |
|
I suppose Jump::Near/Short makes sense. I would be fine with Force5Bytes::Yes/No too, though. |
Replace the bool parameter force5bytes in J, JMP, and J_CC with an enum class Jump::Short/Near. Many callers set that parameter to the literal 'true', which was unclear if you didn't already know what it did.
Dentomologist
force-pushed
the
xemitter_use_pseudonamed_parameter_force5bytes
branch
from
ee108b3
to
4c2759f
Compare
|
I opted for Jump instead of Force5Bytes because J_CC uses 6 bytes instead. |
|
I still think that Short/Near is a weird combination, but they are the official terms (probably from back when farcode was a thing...?) so I guess I'm okay with it. |
Jit64: Fix trampolines after #11834.
Replace the bool parameter force5bytes in J, JMP, and J_CC with an enum class. Many callers set that parameter to the literal 'true', which was unclear if you didn't already know what it did.