recp / cglm

📽 Highly Optimized 2D / 3D Graphics Math (glm) for C
MIT License
2.33k stars 231 forks source link

mat2x3: fix multiplication functions #403

Closed EasyIP2023 closed 7 months ago

EasyIP2023 commented 7 months ago

Believe this is what we want. Per issue: https://github.com/recp/cglm/issues/385

TODO:

recp commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

@recp

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

recp commented 7 months ago

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 commented 7 months ago

@recp

lol

Meant _vmul not _mulv.

EasyIP2023 commented 7 months ago

@recp

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

recp commented 7 months ago

@EasyIP2023 many thanks, the PR is merged 🚀

recp commented 7 months ago

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.