Replace div op with combination of recip and mul #842
Conversation
mrakitaTT
force-pushed
the
mrakita/issue_841
branch
from
d0b2158 to
ec7d32e
Compare
|
Instead of pushing this workaround in the core compiler, can we push priority on the TTNN side? I'm scared that leaving these as P1 will not get fixed soon, and we'll end up using this workaround in the long run. |
@nvukobratTT Can't we do both? :) IMO when the workaround fix is easy to revert it is better to have it in and pursue the proper fix on the side, especially when it is blocking some demos. |
|
@mrakitaTT before workarounding, can we first clarify with TTNN folks the issue and what is needed from their side to support proper solution? After we get feedback from them, we can easily decide whether and how we want to workaround it if necessary. |
|
@sdjordjevicTT The issue is pretty clear and from the conversation under the issue it seems that they are working on it but they can't agree on the proper fix, so I guess it is just a matter of time for them to reach agreement. I can ping them to see if they are close to resolution, though I was not that hopeful after reading full conversation (which I recommend you to do too if you haven't, some wild stuff there). In the meantime, our compiler is producing graph that is failing in runtime in this case. I don't understand why is it the problem in general to have a workaround on our side until they fix their issue? Also the workaround I am using is the one that they suggested in the issue. I would understand if I was introducing some big code change which would be hard to revert later or which affects other parts of the system, but this is literally plug in/plug out change maximally scoped out. I changed to different ConversionPattern and later we just reverse to the old ConversionPattern. Is there something that I am missing, is this going to affect optimization passes or something else? Anyways, I don't really mind if you don't want to have this fix in main, only pain for us currently is that we have to keep it on all our side branches where we test MNIST model, so to ease that pain I wanted to push it into main. I'd just like to understand what is all the fuss about so I can do things different in the future because I think we are going to have many more situations like this 🙂 We are currently dealing with couple of metal issues and judging by their 1.6k open issues it might take a while. |
|
This is an old PR, perhaps we can close? |
|
Issue is over 5 months old closing, please reopen if still an issue! |
|
@nsmithtt if you can believe it, issue still exists :) But new PR should be opened anyways, since we now have a workarounds pass where this should be implemented. I've pinged Metal team in December about this, they moved it to "Done" in January and we actually didn't see an issue anymore, but @vladimirjovanovicTT's team now encountered it in softmax backwards. They will take over adding a workaround. |
|
I opened a new issue regarding div op here: #2350 |
Issue: #841