Initial implementation of implicit broadcasting for eltwise ops #2936

jbobba · 2019-05-15T00:02:58Z

TODO

Figure out the autodiff story for implicit broadcast modes
Will be supported through reductions along broadcast axes in a future PR
Expand to other eltwise ops
Add more eltwise execution unit tests
Serializer support

… Add supported

aprocter

Just comments since this is [WIP] but it looks great so far

src/ngraph/dimension.cpp

src/ngraph/node.cpp

src/ngraph/op/util/binary_elementwise_arithmetic.cpp

src/ngraph/partial_shape.cpp

src/ngraph/op/util/binary_elementwise_arithmetic.cpp

src/ngraph/pass/auto_broadcast.hpp

test/type_prop.cpp

diyessi

Also agree on bcast -> broadcast

src/ngraph/dimension.cpp

src/ngraph/node.cpp

aprocter

Good stuff! In the comments are a couple of cosmetic gripes. A few additional things I think we should do (ideally before merging, but I could be persuaded to take a raincheck):

Expand support to all binops.
Add a few execution tests for binops with implicit broadcast. (Okay to disable in non-CPU/INTERPRETER BEs for now.)

src/ngraph/op/util/attr_types.hpp

jbobba · 2019-05-16T22:16:09Z

Good stuff! In the comments are a couple of cosmetic gripes. A few additional things I think we should do (ideally before merging, but I could be persuaded to take a raincheck):

Expand support to all binops.

Add a few execution tests for binops with implicit broadcast. (Okay to disable in non-CPU/INTERPRETER BEs for now.)

(Likely consequence of 2) Update other backends to invoke.

Will remove WIP once i expand to all binops. We can run execution tests on other backends as well since they will invoke the ImplicitBroadcastElimination pass

aprocter · 2019-05-16T22:17:39Z

Will remove WIP once i expand to all binops. We can run execution tests on other backends as well since they will invoke the ImplicitBroadcastElimination pass

SGTM. I'd actually meant to backspace the comment about updating other BEs, since it occurred to me as I was typing it that the manifest exists. But it might be worth doing in this PR as well.

…imination

aprocter · 2019-05-22T00:35:22Z

src/ngraph/dimension.cpp

+    {
+        // One static. Set dst to static size if >1
+        auto ds = d1.is_dynamic() ? size_t(d2) : size_t(d1);
+        dst = (ds > 1) ? ds : Dimension::dynamic();


I dunno why, but I genuinely love corner cases like this.

rkimballn1 · 2019-05-22T13:58:19Z

src/ngraph/node.cpp

@@ -471,7 +471,8 @@ const NodeVector& ngraph::check_single_output_args(const NodeVector& args)
    return args;
 }

-std::tuple<element::Type, PartialShape> Node::validate_and_infer_elementwise_args()
+std::tuple<element::Type, PartialShape>
+    Node::validate_and_infer_elementwise_args(const op::AutoBroadcastSpec autob)


Pass by reference instead of by value

rkimballn1 · 2019-05-22T14:00:34Z

src/ngraph/op/util/attr_types.hpp

+            NUMPY
+        };
+
+        struct AutoBroadcastSpec


This could use some doc strings, also on ctors

rkimballn1 · 2019-05-22T14:02:09Z

src/ngraph/op/util/attr_types.hpp

+            {
+            }
+
+            AutoBroadcastType type;


I'm sure I am the only one who thinks this, but these should be m_type and there should be accessor functions.

rkimballn1 · 2019-05-22T14:05:39Z

src/ngraph/op/util/attr_types.hpp

+        enum class AutoBroadcastType
+        {
+            NONE = 0,
+            NUMPY


It is a shortcut to just say that this works like numpy broadcast, but it would be nice to actually know the rules without having to go find numpy's documentation to find out how our code works.

rkimballn1 · 2019-05-22T14:06:05Z

src/ngraph/partial_shape.cpp

+
+bool PartialShape::broadcast_merge_into(PartialShape& dst,
+                                        const PartialShape& src,
+                                        const op::AutoBroadcastSpec autob)


pass by reference

rkimballn1 · 2019-05-22T14:09:07Z

src/ngraph/pass/implicit_broadcast_elimination.hpp

+        {
+            if (node->get_autob().type == op::AutoBroadcastType::NONE)
+            {
+                return node->get_arguments();


by having multiple returns you do not allow the compiler to use Return Value Optimization, forcing it to make a copy when returning NodeVector.

This is only used on nodes with two arguments. As such copying might not be too bad. Do you have an alternative in mind?

at the top define NodeVector rc;
instead of returns set rc = <whatever>;
then at the bottom return rc;
One return so RVO works. It is a good general rule to follow to only have one return per function if you are returning something non trivial

rkimballn1 · 2019-05-22T14:10:57Z

src/ngraph/serializer.cpp

+    case OP_TYPEID::Add:
+    {
+        auto tmp = dynamic_cast<const op::Add*>(&n);
+        node["autob"] = write_auto_broadcast(tmp->get_autob());


Would be better if you only wrote out autob if it is not the default value.

nmostafa · 2019-05-22T21:49:33Z

src/ngraph/op/and.cpp

+op::And::And(const shared_ptr<Node>& arg0,
+             const shared_ptr<Node>& arg1,
+             const AutoBroadcastSpec& autob)
+    : BinaryElementwiseLogical("And", arg0, arg1, autob)


Do we need to pass this to every constructor ? I assume autob mode should be fixed for all nodes during at least on ngraph invocation. Can't we instead implicitly read it from some configuration class/global ?

Provide a way to change the default constructor of AutoBroadcastSpec?

It might make sense to have a global that influences the constructors' behavior for the sake of convenience, but I still think we'll need to store autob and respect that stored value, for things like deserialization.

Or set the attribute after construction and before validation.

Basically, is it possible that nodes may have different auto-broadcast modes ? If not, then makes more sense to keep it as a global.

Well, "possible" is up to us, right? ;)

Consider, for example, a builder (either one that's part of Core and meant to be reused across bridges, or one that's used internally by the bridge) that wants to use autobroadcast internally. How would that interact with the global?

nmostafa · 2019-05-22T22:24:12Z

src/ngraph/op/util/attr_types.hpp

+            }
+
+            AutoBroadcastType type;
+            size_t axis;


What is axis ? Couldn't find a use anywhere.

Used to indicate the start of alignment, I added a comment.

rkimballn1 · 2019-05-29T14:06:40Z

src/ngraph/pass/implicit_broadcast_elimination.hpp

+        {
+            if (node->get_autob().type == op::AutoBroadcastType::NONE)
+            {
+                return node->get_arguments();


at the top define NodeVector rc;
instead of returns set rc = <whatever>;
then at the bottom return rc;
One return so RVO works. It is a good general rule to follow to only have one return per function if you are returning something non trivial

diyessi

I don't like passing that ab all over the place, but this will do until we can work something better out like a graph context or something.

diyessi · 2019-05-28T21:12:52Z

src/ngraph/op/and.cpp

+op::And::And(const shared_ptr<Node>& arg0,
+             const shared_ptr<Node>& arg1,
+             const AutoBroadcastSpec& autob)
+    : BinaryElementwiseLogical("And", arg0, arg1, autob)


Or set the attribute after construction and before validation.

Initial implementation of implicit broadcasting for eltwise ops. Only…

dbfc2af

… Add supported

jbobba added the WIP Work In Progress and will not be merged label May 15, 2019

jbobba requested review from aprocter and diyessi May 15, 2019 00:02

jbobba requested review from dmyershov, rkimballn1 and silee2 as code owners May 15, 2019 00:02

aprocter reviewed May 15, 2019

View reviewed changes

diyessi suggested changes May 15, 2019

View reviewed changes

src/ngraph/dimension.cpp Outdated Show resolved Hide resolved

src/ngraph/node.cpp Show resolved Hide resolved

jbobba added 2 commits May 16, 2019 15:01

Addressed PR feedback

f5b8d1f

cleanup

1e008e5

aprocter suggested changes May 16, 2019

View reviewed changes

src/ngraph/op/util/attr_types.hpp Outdated Show resolved Hide resolved

src/ngraph/op/util/attr_types.hpp Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into jbobba/eltwise-autob

85407a4

jbobba added 7 commits May 16, 2019 15:35

Rename Bcast to Broadcast

5f19aea

Autobroadcasting support for rest of elementwise ops

08a2f03

Serializer support for autobroadcast

6aa7c82

Merge remote-tracking branch 'origin/master' into jbobba/eltwise-autob

41858b3

More type prop tests

7254980

Added missing autob serialization for Minimum

09ecf6b

Added execution unit tests and more op types to implicit broadcast el…

5ac79b6

…imination

jbobba changed the title ~~[WIP] Initial implementation of implicit broadcasting for eltwise ops~~ Initial implementation of implicit broadcasting for eltwise ops May 21, 2019

jbobba removed the WIP Work In Progress and will not be merged label May 21, 2019

aprocter approved these changes May 22, 2019

View reviewed changes

rkimballn1 suggested changes May 22, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into jbobba/eltwise-autob

075539c

nmostafa reviewed May 22, 2019

View reviewed changes

Addressed PR feedback

5b4024f

Fixes windows build issue

a34cf08

dmyershov approved these changes May 24, 2019

View reviewed changes

arogowie-intel mentioned this pull request May 24, 2019

[Fused Op] LSTMCell #2966

Merged

postrational added this to the 0.21 milestone May 28, 2019

Merge remote-tracking branch 'origin/master' into jbobba/eltwise-autob

24640ad

jbobba requested review from rkimballn1, diyessi and nmostafa May 28, 2019 20:42

rkimballn1 suggested changes May 29, 2019

View reviewed changes

diyessi approved these changes May 29, 2019

View reviewed changes

nmostafa approved these changes May 29, 2019

View reviewed changes

silee2 approved these changes May 29, 2019

View reviewed changes

jbobba added 2 commits May 30, 2019 13:29

RVO optimization per PR feedback

32de4b3

Merge remote-tracking branch 'origin/master' into jbobba/eltwise-autob

83989db

rkimballn1 approved these changes May 30, 2019

View reviewed changes

rkimballn1 added the Fully Reviewed PR is approved by all reviewers. label May 30, 2019

rkimballn1 merged commit 0caefe7 into master May 30, 2019

rkimballn1 deleted the jbobba/eltwise-autob branch May 30, 2019 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of implicit broadcasting for eltwise ops #2936

Initial implementation of implicit broadcasting for eltwise ops #2936

jbobba commented May 15, 2019 •

edited

aprocter left a comment

diyessi left a comment

aprocter left a comment •

edited

jbobba commented May 16, 2019

aprocter commented May 16, 2019

aprocter May 22, 2019

rkimballn1 May 22, 2019

rkimballn1 May 22, 2019

rkimballn1 May 22, 2019

rkimballn1 May 22, 2019

rkimballn1 May 22, 2019

rkimballn1 May 22, 2019

jbobba May 22, 2019

rkimballn1 May 29, 2019

rkimballn1 May 22, 2019

nmostafa May 22, 2019

jbobba May 22, 2019

aprocter May 22, 2019

diyessi May 28, 2019

nmostafa May 29, 2019

aprocter May 29, 2019

nmostafa May 22, 2019

jbobba May 23, 2019

rkimballn1 May 29, 2019

diyessi left a comment

diyessi May 28, 2019

Initial implementation of implicit broadcasting for eltwise ops #2936

Initial implementation of implicit broadcasting for eltwise ops #2936

Conversation

jbobba commented May 15, 2019 • edited

aprocter left a comment

Choose a reason for hiding this comment

diyessi left a comment

Choose a reason for hiding this comment

aprocter left a comment • edited

Choose a reason for hiding this comment

jbobba commented May 16, 2019

aprocter commented May 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diyessi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbobba commented May 15, 2019 •

edited

aprocter left a comment •

edited