browsermt / marian-dev

Fast Neural Machine Translation in C++ - development repository
https://marian-nmt.github.io
Other
20 stars 7 forks source link

Replace "dynamic" loop condition with const variable #73

Closed jelmervdl closed 2 years ago

jelmervdl commented 2 years ago

Fixes #72.

I presume this helps the compiler with realising that the condition doesn't change, and allows it to unroll/optimise it much better.

graemenail commented 2 years ago

Dumb question, is rows(item.shape) x cols(item.shape) here just item.shape.elements()?

XapaJIaMnu commented 2 years ago

@graemenail basically: https://github.com/browsermt/marian-dev/blob/ab3c33af65bbf47a2c9dcf47729af857fe234806/src/tensors/cpu/integer_common.h#L23

graemenail commented 2 years ago

Can we use .elements() @jelmervdl ?

row/cols helpers take Shape by non-const ref

XapaJIaMnu commented 2 years ago

Can we use .elements() @jelmervdl ?

row/cols helpers take Shape by non-const ref

tbh, that is my fault here, as I reused these helpers from some things that were implemented back in 2018 and never paid much attention to them. Should I rework them to only take const reference, or make them part of the tensor class and mark them as const methods?

graemenail commented 2 years ago

I feel they should probably live up in Shape. But TensorBase doesn't have a const getter for shape_, so we'd need to add that as well.

graemenail commented 2 years ago

Even virtual size_t size() { return shape_.elements(); } isn't const in TensorBase

XapaJIaMnu commented 2 years ago

Sounds like something to worth fixing upstream. I use rows() and cols() several times during each gemm operation, not sure if hidden cost appears there.

graemenail commented 2 years ago

Indeed, as most tensors ops start with something like:

int rows = x->shape().elements() / x->shape().back();
int cols = x->shape().back();
XapaJIaMnu commented 2 years ago

Since in the long term, we plan to move to using upstream, I say we merge the quick fixes here and not worry about refactoring the graph?

graemenail commented 2 years ago

Agreed