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

[ONNX] only broadcast matmul if the shape has changed #10321

Merged
merged 2 commits into from
Feb 22, 2022

Conversation

mbrookhart
Copy link
Contributor

cc @AndrewZhaoLuo @jwfromm

#9911 introduced some extra broadcasting to handle edge cases in shapes. In cases where the broadcasted shape matched the input shape, this ended up being a null op that caused some issues with a model I've been testing.

I think the issue is deeper in how broadcast_to is lowered, but removing the unnecessary broadcast also fixes my bug, so I'm punting and adding the deeper issue to my backlog.

Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jwfromm jwfromm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lowering to a null op is very interesting behavior. We should make sure to fix that eventually.. This work around in the meantime makes sense to me.

@AndrewZhaoLuo
Copy link
Contributor

Can we make a tracking issue for this?

@jwfromm
Copy link
Contributor

jwfromm commented Feb 18, 2022

We might have to poke around a little more to figure out the issue. I wasn't able to replicate a failure using a dedicated script with useless broadcast_to calls inserted. This seems to run fine for example:

import numpy as np
import tvm
from tvm import relay
from tvm.contrib import graph_executor

x = relay.var('x', shape=[10], dtype='float32')
y = relay.var('y', shape=[10], dtype='float32')
val = relay.const(5, dtype='float32')
x2 = relay.broadcast_to(x, relay.shape_of(y))
out = x2 + val
mod = tvm.IRModule.from_expr(out)

with relay.build_config(opt_level=3):
    lib = relay.build(mod, target="llvm")

gmod = graph_executor.GraphModule(lib["default"](tvm.cpu()))
x_np = np.random.normal(size=[10]).astype('float32')
y_np = np.random.normal(size=[10]).astype('float32')
gmod.set_input('x', x_np)
gmod.set_input('y', y_np)
gmod.run()
print(gmod.get_output(0))

Do we expect that to trigger the issue or is it more nuanced?

@mbrookhart
Copy link
Contributor Author

I haven't been able to isolate the problem from the model yet, but we'll work to do that offline, reproduce as a test, and then file the issue

@mbrookhart mbrookhart merged commit 5956125 into apache:main Feb 22, 2022
@mbrookhart
Copy link
Contributor Author

Thanks @AndrewZhaoLuo @jwfromm

pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request Apr 11, 2022
* [ONNX] only broadcast matmul if the shape has changed

* fix copy-pasta mistake
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.

3 participants