Mhc optimization: Rewrites einsums for better xla fusion - #4664
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| ) | ||
| layer_input = jnp.einsum("bskd,bsk -> bsd", x, pre_mapping, precision=self.matmul_precision) | ||
| # bskd, bsk -> bsd | ||
| layer_input = jnp.sum(x * jnp.expand_dims(pre_mapping, axis=3), axis=2) |
There was a problem hiding this comment.
for my understanding, do you see perf gain by this change? I thought einsum would be more performant.
There was a problem hiding this comment.
Yes, even I found it a bit counter-intuitive einsum being more performant is what my understanding was as well. I think while being generally true for some cases like this one it seems better optimization is coming by moving away from it. Specifically, I can see the all-reduce overlap. I tested the changes individually by just changing the einsums and for_iloop but that does not seem to work I'm only seeing the All-Reduce overlap when both the changes are present.
Just for_iloop replaced :

Just for_iloop and einsum replaced:

I don't have a very solid reasoning behind why of it.
Description
XLA optimizations does not seem to gel well with einsum for cases where dimensions are being expanded. Leading to a ~10- 15% performance drop in the current implementation. This change implements expanded einsums with jnp.expand_dims leading to better XLA fusion.
Before (110us):


After (97us):
Tests
All existing tests passing.
Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.