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

More shader refactoring #2346

Merged
merged 15 commits into from Jan 27, 2017

Conversation

Projects
None yet
7 participants
@yuriks
Member

yuriks commented Dec 17, 2016

Act 2: In which I give @JayFoxRox a heart attack.

I strongly recommend reviewing each commit individually. There's a lot of stuff here, but the common theme is removing direct dependencies on global state.

@JayFoxRox: Most of this seems to me like it should make your job easier, since it makes it trivial to have several independent shader units. If you think it's too onerous to rebase on top of this, I can split some of these changes out to a separate PR I can rebase on top of your stuff whenever you eventually submit that. (Though I'd of course prefer to merge my cleanups now...)

(As a bonus, this also fixes #1051)

@JayFoxRox

This comment has been minimized.

Show comment
Hide comment
@JayFoxRox

JayFoxRox Dec 17, 2016

Member

It will take some time to review this. I'll try to rebase my GS branch onto it to see if there are any design issues.

I believe this fixes #1051 by moving the check for the enabled JIT to the GetEngine() which will remain good even if the setting is changed shortly after. I don't think it fixes it anymore, even if it does we can review it later.

Member

JayFoxRox commented Dec 17, 2016

It will take some time to review this. I'll try to rebase my GS branch onto it to see if there are any design issues.

I believe this fixes #1051 by moving the check for the enabled JIT to the GetEngine() which will remain good even if the setting is changed shortly after. I don't think it fixes it anymore, even if it does we can review it later.

Show outdated Hide outdated src/video_core/shader/shader_jit_x64_compiler.cpp
@@ -833,7 +824,11 @@ void JitShader::FindReturnOffsets() {
std::sort(return_offsets.begin(), return_offsets.end());
}
void JitShader::Compile() {
void JitShader::Compile(const std::array<u32, 1024>* program_code_,
const std::array<u32, 1024>* swizzle_data_) {

This comment has been minimized.

@JayFoxRox

JayFoxRox Dec 17, 2016

Member

I'm not sure wether VS and GS uniforms are different (per shader type), but if they are, this should just get passed the ShaderSetup which only consists of Uniforms, Program Code and Swizzle Data. (so if uniforms are also different this would essentially just unpack and repack a ShaderSetup).
Even if the uniforms are different per shader unit this could be passed a ShaderSetup as we'd move the uniforms to the unit state instead. (so program_code and swizzle_data would be everything the ShaderSetup consists of).

@JayFoxRox

JayFoxRox Dec 17, 2016

Member

I'm not sure wether VS and GS uniforms are different (per shader type), but if they are, this should just get passed the ShaderSetup which only consists of Uniforms, Program Code and Swizzle Data. (so if uniforms are also different this would essentially just unpack and repack a ShaderSetup).
Even if the uniforms are different per shader unit this could be passed a ShaderSetup as we'd move the uniforms to the unit state instead. (so program_code and swizzle_data would be everything the ShaderSetup consists of).

This comment has been minimized.

@yuriks

yuriks Dec 17, 2016

Member

Different in the sense that they have different structure, or different values? They definitely have different values (otherwise it'd be impossible to decouple shader programs from each other because, for example, all shader constants are actually uniforms.) The compiler doesn't need to should know about uniform values, so I only pass to it the things it's actually compiling (the program/swizzles.)

@yuriks

yuriks Dec 17, 2016

Member

Different in the sense that they have different structure, or different values? They definitely have different values (otherwise it'd be impossible to decouple shader programs from each other because, for example, all shader constants are actually uniforms.) The compiler doesn't need to should know about uniform values, so I only pass to it the things it's actually compiling (the program/swizzles.)

Show outdated Hide outdated src/video_core/shader/shader.cpp
}
#endif // ARCHITECTURE_x86_64
conditional_code[0] = false;
conditional_code[1] = false;

This comment has been minimized.

@JayFoxRox

JayFoxRox Dec 17, 2016

Member

Why is this part of vertex loading?

@JayFoxRox

JayFoxRox Dec 17, 2016

Member

Why is this part of vertex loading?

This comment has been minimized.

@yuriks

yuriks Dec 17, 2016

Member

Moved into the interpreter. (The JIT didn't use them at all, I was tempted to just remove them from the struct entirely.)

@yuriks

yuriks Dec 17, 2016

Member

Moved into the interpreter. (The JIT didn't use them at all, I was tempted to just remove them from the struct entirely.)

Show outdated Hide outdated src/video_core/shader/shader.cpp
#ifdef ARCHITECTURE_x86_64
// TODO(yuriks): Re-initialize on each change rather than being persistent

This comment has been minimized.

@JayFoxRox

JayFoxRox Dec 17, 2016

Member

We need at least 2 engines.

When GS is enabled, the VS will be run a couple of times until a buffer is filled up.
Once the buffer has filled up to a certain point the GS will process the generated data.
After that the VS will continue its thing.

As we currently only have 1 engine which is modified by SetupBatch its not possible to switch code back and forth (between VS / GS). We currently don't know if all 4 shader units can have entirely different code / uniform data or if it's just per shader type (VS / GS).

@JayFoxRox

JayFoxRox Dec 17, 2016

Member

We need at least 2 engines.

When GS is enabled, the VS will be run a couple of times until a buffer is filled up.
Once the buffer has filled up to a certain point the GS will process the generated data.
After that the VS will continue its thing.

As we currently only have 1 engine which is modified by SetupBatch its not possible to switch code back and forth (between VS / GS). We currently don't know if all 4 shader units can have entirely different code / uniform data or if it's just per shader type (VS / GS).

This comment has been minimized.

@yuriks

yuriks Dec 17, 2016

Member

I envisioned engines as being singletons, because that way they can share cache. I'll try to think of an alternative way of allowing multiple concurrent setups.

@yuriks

yuriks Dec 17, 2016

Member

I envisioned engines as being singletons, because that way they can share cache. I'll try to think of an alternative way of allowing multiple concurrent setups.

This comment has been minimized.

@jroweboy

jroweboy Jan 26, 2017

Member

Just following up on this conversation (since its the last call for reviews) @yuriks any thoughts on how to have multiple engines yet?

@jroweboy

jroweboy Jan 26, 2017

Member

Just following up on this conversation (since its the last call for reviews) @yuriks any thoughts on how to have multiple engines yet?

This comment has been minimized.

@yuriks

yuriks Jan 26, 2017

Member

Yes, this has been addressed by "VideoCore/Shader: Move per-batch ShaderEngine state into ShaderSetup"

@yuriks

yuriks Jan 26, 2017

Member

Yes, this has been addressed by "VideoCore/Shader: Move per-batch ShaderEngine state into ShaderSetup"

@JayFoxRox

This comment has been minimized.

Show comment
Hide comment
@JayFoxRox

JayFoxRox Dec 17, 2016

Member

I've rebased my GS branch JayFoxRox#49

Problems with design:

  1. We need 2 engines (/ cached shader code) at the same time
  2. Input vertex loading for GS is done from a buffer, not InputVertex, so the LoadInputVertex function is kind of useless here (we could overload it maybe?)
  3. Initialization of the conditional vars must be done manually currently as GS doesn't use LoadInputVertex

Aside from that it's fine I think. I only tested 2 games though.
I also did not rebase my shader debugger yet, but I guess it's not that important.

Member

JayFoxRox commented Dec 17, 2016

I've rebased my GS branch JayFoxRox#49

Problems with design:

  1. We need 2 engines (/ cached shader code) at the same time
  2. Input vertex loading for GS is done from a buffer, not InputVertex, so the LoadInputVertex function is kind of useless here (we could overload it maybe?)
  3. Initialization of the conditional vars must be done manually currently as GS doesn't use LoadInputVertex

Aside from that it's fine I think. I only tested 2 games though.
I also did not rebase my shader debugger yet, but I guess it's not that important.

CompiledShader* program = nullptr;
};
} // Shader

This comment has been minimized.

@lioncash

lioncash Dec 17, 2016

Member

Isn't the general convention to do // namespace Shader as opposed to just the name of the namespace?

@lioncash

lioncash Dec 17, 2016

Member

Isn't the general convention to do // namespace Shader as opposed to just the name of the namespace?

This comment has been minimized.

@yuriks

yuriks Dec 17, 2016

Member

I guess? I didn't change this line.

@yuriks

yuriks Dec 17, 2016

Member

I guess? I didn't change this line.

This comment has been minimized.

@lioncash

lioncash Dec 17, 2016

Member

Ah OK, I misread the diff then, my bad.

@lioncash

lioncash Dec 17, 2016

Member

Ah OK, I misread the diff then, my bad.

This comment has been minimized.

@yuriks

yuriks Dec 18, 2016

Member

This was part of a file rename, and then I created another file with the same name on a later commit. So it doesn't get detected by the full diff, but if you look at each individual commit then it doesn't show up as a change.

@yuriks

yuriks Dec 18, 2016

Member

This was part of a file rename, and then I created another file with the same name on a later commit. So it doesn't get detected by the full diff, but if you look at each individual commit then it doesn't show up as a change.

@yuriks

This comment has been minimized.

Show comment
Hide comment
@yuriks

yuriks Dec 17, 2016

Member

@JayFoxRox

We need 2 engines (/ cached shader code) at the same time

I'm trying to avoid that. Will think about how to solve this.

Input vertex loading for GS is done from a buffer, not InputVertex, so the LoadInputVertex function is kind of useless here (we could overload it maybe?)

LoadInputVertex doesn't apply then, just make another function to transfer VSH outputs to GSH inputs. We'll probably end up with a bit of code duplication. For example, I think the way we're handling the VSH outputs right now merges two concepts that should be separate (shader engine appending output regs into a buffer/queue according to the outmask, and the geometry unit reading them from that buffer), but that can be refactored after we have the two shader types in place.

Initialization of the conditional vars must be done manually currently as GS doesn't use LoadInputVertex

Fixed that.

Member

yuriks commented Dec 17, 2016

@JayFoxRox

We need 2 engines (/ cached shader code) at the same time

I'm trying to avoid that. Will think about how to solve this.

Input vertex loading for GS is done from a buffer, not InputVertex, so the LoadInputVertex function is kind of useless here (we could overload it maybe?)

LoadInputVertex doesn't apply then, just make another function to transfer VSH outputs to GSH inputs. We'll probably end up with a bit of code duplication. For example, I think the way we're handling the VSH outputs right now merges two concepts that should be separate (shader engine appending output regs into a buffer/queue according to the outmask, and the geometry unit reading them from that buffer), but that can be refactored after we have the two shader types in place.

Initialization of the conditional vars must be done manually currently as GS doesn't use LoadInputVertex

Fixed that.

@yuriks

This comment has been minimized.

Show comment
Hide comment
@yuriks

yuriks Dec 18, 2016

Member

@JayFoxRox Pushed a commit which should address the issue with using 2 different shaders at the same time.

Member

yuriks commented Dec 18, 2016

@JayFoxRox Pushed a commit which should address the issue with using 2 different shaders at the same time.

@yuriks

This comment has been minimized.

Show comment
Hide comment
@yuriks

yuriks Dec 18, 2016

Member

Assuming the last changes address @JayFoxRox's concerns, I'll cut this PR off here. I have more refactors I want to make but I want to avoid the PR growing too large.

Member

yuriks commented Dec 18, 2016

Assuming the last changes address @JayFoxRox's concerns, I'll cut this PR off here. I have more refactors I want to make but I want to avoid the PR growing too large.

@JayFoxRox

This comment has been minimized.

Show comment
Hide comment
@JayFoxRox

JayFoxRox Dec 19, 2016

Member

LGTM

Member

JayFoxRox commented Dec 19, 2016

LGTM

@yuriks

This comment has been minimized.

Show comment
Hide comment
@yuriks

yuriks Jan 4, 2017

Member

FYI I'm waiting on @JayFoxRox to do another review on this one (and possibly someone else if they feel like they know the code enough) because that last one had been when he'd been up for more than 24 hours or something... After that I'll rebase and merge.

Member

yuriks commented Jan 4, 2017

FYI I'm waiting on @JayFoxRox to do another review on this one (and possibly someone else if they feel like they know the code enough) because that last one had been when he'd been up for more than 24 hours or something... After that I'll rebase and merge.

@wwylele wwylele added pr:needs-review and removed pr:reviewed labels Jan 9, 2017

@yuriks yuriks removed the pr:conflicted label Jan 19, 2017

@yuriks

This comment has been minimized.

Show comment
Hide comment
@yuriks

yuriks Jan 19, 2017

Member

Rebased. Since @JayFoxRox seems to still be MIA, I'd love if someone else could review this. Otherwise I'll just merge it in say, a week.

Member

yuriks commented Jan 19, 2017

Rebased. Since @JayFoxRox seems to still be MIA, I'd love if someone else could review this. Otherwise I'll just merge it in say, a week.

Show outdated Hide outdated src/video_core/shader/shader_jit_x64_compiler.cpp
void JitShader::Compile() {
void JitShader::Compile(const std::array<u32, 1024>* program_code_,
const std::array<u32, 1024>* swizzle_data_) {
program_code = program_code_;

This comment has been minimized.

@Subv

Subv Jan 22, 2017

Member

What are these members for? You assign them here but set them to nullptr at the end of the function

@Subv

Subv Jan 22, 2017

Member

What are these members for? You assign them here but set them to nullptr at the end of the function

This comment has been minimized.

@JayFoxRox

JayFoxRox Jan 22, 2017

Member

They are used in the compilation functions (one per instruction) and are not passed by parameter. So in order to make them available they are temporarily stored in members during compilation.

As we don't need them afterwards anymore we can set them to nullptr afterwards.

@JayFoxRox

JayFoxRox Jan 22, 2017

Member

They are used in the compilation functions (one per instruction) and are not passed by parameter. So in order to make them available they are temporarily stored in members during compilation.

As we don't need them afterwards anymore we can set them to nullptr afterwards.

This comment has been minimized.

@yuriks

yuriks Jan 23, 2017

Member

It's as he says. Not the best design, but unfortunately doing otherwise would require passing those parameters around on every function call.

@yuriks

yuriks Jan 23, 2017

Member

It's as he says. Not the best design, but unfortunately doing otherwise would require passing those parameters around on every function call.

Show outdated Hide outdated src/video_core/shader/shader.h
@@ -177,13 +183,14 @@ class ShaderEngine {
* Performs any shader unit setup that only needs to happen once per shader (as opposed to once
* per vertex, which would happen within the `Run` function).
*/
virtual void SetupBatch(const ShaderSetup* setup) = 0;
virtual void SetupBatch(ShaderSetup& setup) = 0;
/**
* Runs the currently setup shader
* @param state Shader unit state, must be setup per shader and per shader unit

This comment has been minimized.

@Subv

Subv Jan 22, 2017

Member

This comment is outdated now

@Subv

Subv Jan 22, 2017

Member

This comment is outdated now

This comment has been minimized.

@yuriks

yuriks Jan 23, 2017

Member

Updated.

@yuriks

yuriks Jan 23, 2017

Member

Updated.

@bunnei

bunnei approved these changes Jan 26, 2017

@Subv

Subv approved these changes Jan 27, 2017

@wwylele wwylele added pr:reviewed and removed pr:needs-review labels Jan 27, 2017

@yuriks yuriks merged commit bf14f4b into citra-emu:master Jan 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yuriks yuriks deleted the yuriks:shader-refactor2 branch Jan 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment