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

mat2x3: fix multiplication functions #403

Merged
merged 2 commits into from
Mar 31, 2024

Conversation

EasyIP2023
Copy link
Contributor

@EasyIP2023 EasyIP2023 commented Mar 30, 2024

Believe this is what we want. Per issue: #385

TODO:

  • Update docs
  • Re-review comments
  • Re-review tests

@EasyIP2023 EasyIP2023 force-pushed the bugfix/mat2x3-multiplication branch 3 times, most recently from 74232c4 to ca389f1 Compare March 31, 2024 00:20
@recp
Copy link
Owner

recp commented Mar 31, 2024

Hi @EasyIP2023 ,

Thanks for fixing it. I had forgotten them :(

 float a00 = m1[0][0], a10 = m1[1][0],
       a01 = m1[0][1], a11 = m1[1][1],
       a02 = m1[0][2], a12 = m1[1][2],

You are accessing memory randomly which is not desired in cglm. Compiler may optimize it but compilers may not have same capabilities

Desired access:

  float a00 = m1[0][0], a01 = m1[0][1], a02 = m1[0][2],
        a10 = m1[1][0], a11 = m1[1][1], a12 = m1[1][2]
  COL0: a00 = m1[0][0], a01 = m1[0][1], a02 = m1[0][2],
  COL1: a10 = m1[1][0], a11 = m1[1][1], a12 = m1[1][2]

Cache locality is important even compiler may do this for us, given hint is desired style. Same for setting dest[]:

dest[0][0] = ...
dest[0][1] = ...

dest[1][0] = ...
dest[1][1] = ...

Thanks

@EasyIP2023
Copy link
Contributor Author

@recp Now it makes perfect since why. Okay Changing now!

Do you think I should also do the same in _mulv and _mulv?

Or would assigning variables then do math take longer then accessing elements randomly and do math?

@EasyIP2023 EasyIP2023 force-pushed the bugfix/mat2x3-multiplication branch from ca389f1 to 8f6f568 Compare March 31, 2024 01:26
@EasyIP2023
Copy link
Contributor Author

@recp

Just read other message will remove _mulv. Until more feedback given.

@recp
Copy link
Owner

recp commented Mar 31, 2024

Do you think I should also do the same in _mulv and _mulv?

I'm not sure if there is a benefit to move vector on stack but not matrix, access both on array or both on stack I think but not sure. Probably compiler will do the job but asm output can be compared.

Coding style:

Declare first then init pls. ( except array init, const init )

float v0 = v[0], v1 = v[1];

to

float v0, v1;

v0 = v[0];
v1 = v[1];

for similar places...

Just read other message will remove _mulv. Until more feedback given.

No, _mulv remain exist just waiting existence of _vmul.

@EasyIP2023 EasyIP2023 force-pushed the bugfix/mat2x3-multiplication branch from 8f6f568 to 6aed774 Compare March 31, 2024 01:41
@EasyIP2023
Copy link
Contributor Author

@recp

lol

Meant _vmul not _mulv.

@EasyIP2023 EasyIP2023 force-pushed the bugfix/mat2x3-multiplication branch from 6aed774 to 79129d9 Compare March 31, 2024 01:56
Signed-off-by: Vincent Davis Jr <vince@underview.tech>
This also includes tables to explain how
mat2x3, column vectors and row vectors are
represented. Also includes how resulting
matrix or vector is formed.

Signed-off-by: Vincent Davis Jr <vince@underview.tech>
@EasyIP2023 EasyIP2023 force-pushed the bugfix/mat2x3-multiplication branch from 79129d9 to c5dcb93 Compare March 31, 2024 02:08
@EasyIP2023
Copy link
Contributor Author

@recp

This branch should be all cleaned up and ready for another review now.

@recp recp merged commit c9adbaa into recp:master Mar 31, 2024
74 checks passed
@recp
Copy link
Owner

recp commented Mar 31, 2024

@EasyIP2023 many thanks, the PR is merged 🚀

@recp
Copy link
Owner

recp commented Mar 31, 2024

EDIT:

float v0 = v[0], v1 = v[1];

This seems better fit existing code style, especially reading items from matrices, vectors... My actual intention was more general. Maybe variables that are not reading func params.

@EasyIP2023 EasyIP2023 deleted the bugfix/mat2x3-multiplication branch March 31, 2024 15:05
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