Skip to content
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

D3d12: Code cleaning #1276

Merged
merged 18 commits into from Oct 30, 2015

Conversation

Projects
None yet
5 participants
@vlj
Copy link
Contributor

commented Oct 29, 2015

This PR doesn't bring any functionnality or performance improvement.

It cleans code by fixing code style (use underscore function/variable name), add some no except and split functions so that code is more readable.

@vlj vlj force-pushed the vlj:d3d12 branch from 45756b0 to e3bd8a7 Oct 29, 2015

// Assume 64 bytes cache line
int offset = 0;
bool isAligned = !((size_t)src & 15);
#pragma omp parallel for

This comment has been minimized.

Copy link
@danilaml

danilaml Oct 29, 2015

Contributor

btw does OMP really bring any improvements here? I remember there was an issue with rpcs3 having lots of thread already. Or was that in another place?

This comment has been minimized.

Copy link
@vlj

vlj Oct 29, 2015

Author Contributor

no it doesn't, it's a left over pragma, thanks for pointing it !
(Although it shouldn't matter since Open mp flag is not set in any project)

@Nekotekina

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

Let's fix lack of newlines at the end of file for consistency. Btw, added new rule on wiki.

@vlj vlj force-pushed the vlj:d3d12 branch from e3bd8a7 to 76797cd Oct 29, 2015

@vlj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2015

Removed the useless omp pragma and added newline at the end of every d3d12 related files.

@vlj vlj force-pushed the vlj:d3d12 branch from 76797cd to 4f71074 Oct 29, 2015

vlj added some commits Oct 29, 2015

d3d12: Add an extra varying.
This fixes shader compilation for SH3 HD.

@vlj vlj force-pushed the vlj:d3d12 branch from 57eb55f to a2f7f37 Oct 29, 2015

@Nekotekina

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

I don't like this unreachable thing. If I understood it correctly, If the compiler assumes that default case is unreachable, and any invalid value is taken from method_registers, the result is unpredictable. Plus it doesn't match upper-case macro style.

@vlj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2015

rpcs3 doesn't define NDEBUG even in release mode, so unreachable just calls abort and can provide line number and file name when it crashes.
If we ever define NDEBUG yes the compiler will assume that this code is not reachable and thus will optimise the switch (possibly turning it into a non switch statement) but it's rather for performance build rather than debug one and I don't think we will provide a performance build soon.
It suppresses the "control reaches non void" warning emitted by several compilers.

It's used everywhere in llvm (that's where I took the code snippet) as "abort" and "assert(0)" is considered there as not bringing enough information and occuring an unecessarry if statement in code for performance build.

vlj added some commits Oct 30, 2015

@vlj vlj force-pushed the vlj:d3d12 branch from d218c93 to 449c41a Oct 30, 2015

B1ackDaemon pushed a commit that referenced this pull request Oct 30, 2015

B1ackDaemon
Merge pull request #1276 from vlj/d3d12
D3d12: Code cleaning

@B1ackDaemon B1ackDaemon merged commit b5cf7fb into RPCS3:master Oct 30, 2015

3 checks passed

code-review/pullapprove Approved by DHrpcs3
Details
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Nekotekina

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

Did you test it? How it's supposed to provide info via the abort call?
I don't say that assert(0) is better. I just say that "unreachable" looks like a time bomb for me.

@Nekotekina

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

Ideally, that "__assume" thing can only be used if you can guarantee that "unreachable code" is truly unreachable. But it's possible only if you have complete control over it, which isn't the case when you take values from method registers.

@Zangetsu38

This comment has been minimized.

Copy link

commented on 119126c Oct 30, 2015

C'est ce commit la qui fait crash Silent 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.