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

[spv-out] Add multiply instructions #219

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

Timo-DK
Copy link
Collaborator

@Timo-DK Timo-DK commented Oct 1, 2020

Add support for the long-awaited multiply instructions:

  • OpIMul
  • OpFMul
  • OpVectorTimesScalar
  • OpVectorTimesMatrix
  • OpMatrixTimesVector
  • OpMatrixTimesScalar
  • OpMatrixTimesMatrix

Tested with the following WGSL:

[[stage(fragment)]]
fn main() -> void {
    # OpIMul
    var int_1: i32 = 1;
    var int_2: i32 = int_1 * int_1;

    # OpFMul
    var float_1: f32 = 1.0;
    var float_2: f32 = float_1 * float_1;

    # OpVectorTimesScalar
    var vec_1: vec2<f32> = vec2<f32>(1.0, 1.0);
    var vec_2: vec2<f32> = vec_1 * float_1;

    # OpVectorTimesMatrix
    var mat_1: mat2x2<f32> = mat2x2<f32>(1.0, 1.0);
    var vec_3: vec2<f32> = vec_2 * mat_1;

    # OpMatrixTimesVector
    var vec_4: vec2<f32> = mat_1 * vec_3;

    # OpMatrixTimesScalar
    var mat_2: mat2x2<f32> = mat_1 * float_2;

    # OpMatrixTimesMatrix
    var mat_3: mat2x2<f32> = mat_2 * mat_2;

    return;
}

@Timo-DK Timo-DK force-pushed the spv-out-multiply-support branch from c9e4ffd to aa98d5f Compare October 3, 2020 20:26
@Timo-DK Timo-DK marked this pull request as ready for review October 3, 2020 20:28
@Timo-DK Timo-DK requested a review from kvark October 3, 2020 20:28
src/back/spv/instructions.rs Show resolved Hide resolved
src/back/spv/instructions.rs Outdated Show resolved Hide resolved
let left_load_id = self.generate_id();
let right_load_id = self.generate_id();

block.body.push(super::instructions::instruction_load(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are there load instructions here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, because the values are from an OpVariable for instance, and to use an OpVariable, you have to load them through OpLoad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, but there isn't any check here about whether the target is OpVariable or not. So how do we know that unconditionally "load"-ing is the right thing to do?

Copy link
Collaborator Author

@Timo-DK Timo-DK Oct 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true, but as far as I know there are only two ways to use binary instructions at high level: via pointers, and via constants.

I have done some research at http://shader-playground.timjones.io/, and when doing it via pointers, there is always an OpLoad needed.

GLSL:

void main()
{
    vec2 vec_1 = vec2(1.0, 1.0);
    float float_1 = vec_1.x * vec_1.y;
    float float_3 = float_1 * float_1;
}

This will always translate back to OpLoad, whether it is via an OpAccessChain or OpVariable. There probably are more ways of accessing value via pointers, but all pointers always need to be loaded in via OpLoad to be useful (as far as I know).

What is good to see, is that when I am using constants, instead of variables at the shader playground, it pre-calculates the value, and uses it directly (which is allowed).

GLSL:

void main()
{
    float float_1 = 1.0 * 2.0;
}

However, because of the pre-calculation, the resulting SPIR-V output does not even contain a binary instruction, as it is already calculated.

I think it is safe to say for now to always use OpLoad here, but maybe you think about a case where this might not be, please let me know, so we can reconsider this piece of code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't always use OpLoad there. It should only happen if the target expression is a variable. Once it's a different expression, there should be no load. For example, if you do float x = 2 * (y + 1);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I understand what you mean. I have a rough idea, and will try to implement it tomorrow: what if we never check for the specific Expression::LocalVariable, but let the match arm of Expression::LocalVariable always create and return an OpLoad?

That should fix this issue, and the rest of the code will be cleaner, as Expression::Return, and Expression::Store have a specific match for if the id of the expression is of the expression type Expression::LocalVariable. This is pretty dirty to be honest, but I didn't see that back then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, both LocalVariable and GlobalVariable variants need to generate OpLoad.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and now it supports even "unlimited" expression nesting:

var float_1: f32 = 2.0 * (2.0 * (4.0 * 2.0));

Giving the predicted result without an OpLoad, like you said (and correctly validates):

%5 = OpVariable %_ptr_Function_float Function
%13 = OpFMul %float %float_4 %float_2
%12 = OpFMul %float %float_2 %13
%11 = OpFMul %float %float_2 %12
      OpStore %5 %11 None

@Timo-DK Timo-DK force-pushed the spv-out-multiply-support branch from aa98d5f to e6d2021 Compare October 3, 2020 22:38
src/back/spv/instructions.rs Show resolved Hide resolved
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one concern

let left_load_id = self.generate_id();
let right_load_id = self.generate_id();

block.body.push(super::instructions::instruction_load(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, but there isn't any check here about whether the target is OpVariable or not. So how do we know that unconditionally "load"-ing is the right thing to do?

@Timo-DK Timo-DK force-pushed the spv-out-multiply-support branch from e6d2021 to af02f49 Compare October 6, 2020 18:59
src/back/spv/writer.rs Show resolved Hide resolved
@kvark kvark merged commit f67dfd2 into gfx-rs:master Oct 6, 2020
@Timo-DK Timo-DK deleted the spv-out-multiply-support branch October 6, 2020 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants