[GraphOptimizer] Extend Reshape sinking pass for binary eltwise ops #5715

aturetsk · 2021-06-10T19:48:05Z

Summary:
Extend Reshape sinking pass for binary eltwise ops in GraphOptimizer.

Documentation:
N/A

Test Plan:
Added GraphOptz unit test.

aturetsk · 2021-06-10T19:49:16Z

@jfix71, continuing effort on adding generic sinking logic started in #5616.

stale · 2021-07-01T21:08:16Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

aturetsk · 2021-07-01T21:18:43Z

Ping

aturetsk · 2021-07-12T20:17:44Z

Ping.

jfix71

Makes sense, but added some comments about making it general, and less complicated.

jfix71 · 2021-07-16T20:33:32Z

tools/ClassGen/NodeGen.cpp

@@ -1432,7 +1431,6 @@ int main(int argc, char **argv) {
      .addInput("Input")
      .addInput("Mapping")
      .addResultFromCtorArg()
-      .dataParallel()


Why is this necessary?

IntLookupTable is not really a data parallel operation. We need the whole Mapping input in order to produce a single output element.

jfix71 · 2021-07-16T20:59:16Z

tools/ClassGen/NodeGen.cpp

      .setDocstring("Selects indices of the true elements in Cond");
+  // clang-format on


nit: Assuming you're doing this because NonZero is seeing weird formatting when dataParallel is removed -- can we keep the original // clang-format on after Exp and add // clang-format off just above NonZero here? Otherwise we're wrapping Logit too unnecessarily..

jfix71 · 2021-07-16T21:22:17Z

lib/Optimizer/GraphOptimizer/GraphOptimizer.cpp

-    if (!node->isDataParallel() || node->hasSideEffects()) {
-      continue;
-    }
+    if (node->isDataParallel() && !node->hasSideEffects()) {


Why can't we leave this as an early continue? Looks like there's no logic after the if.

The plan is to extend this pass further with other nodes (non data parallel). There is more reshape sinking code in Cadence we're planning to upstream eventually, plus I was planning to move existing reshape related code from sinkCode here.

jfix71 · 2021-07-16T21:48:42Z

lib/Optimizer/GraphOptimizer/GraphOptimizer.cpp

+        // Create new Constant, Quantize or Splat, if needed.
+        NodeValue rhs;
+        if (RRN) {
+          rhs = RRN->getInput();
+        }
+        if (RC) {
+          auto ty = F->getParent()->uniqueTypeWithNewShape(
+              RC->getType(), LRN->getInput().dims());
+          auto *newC = F->getParent()->createConstant(ty, RC->getName());
+          newC->getPayloadMutable().copyRawFrom(&RC->getPayload());
+          rhs = newC->getOutput();
+        }
+        if (RQ) {
+          auto ty = F->getParent()->uniqueTypeWithNewShape(
+              RQ->getResult().getType(), LRN->getInput().dims());
+          rhs = F->createQuantize(RQ->getName(), rhs, ty);
+        }
+        if (RS) {
+          auto ty = F->getParent()->uniqueTypeWithNewShape(
+              RS->getResult().getType(), LRN->getInput().dims());
+          rhs = F->createSplat(RS->getName(), ty, RS->getValue());
+        }


All this code seems unnecessarily complex. We have optimizations that do folding of Reshapes into Constants/Splats, and if we have a Constant -> Quantize -> Reshape then it'll get constant folded too. Can we not just add a Reshape here and let other optimizations clean it up later? E.g.

Suggested change

// Create new Constant, Quantize or Splat, if needed.

NodeValue rhs;

if (RRN) {

rhs = RRN->getInput();

}

if (RC) {

auto ty = F->getParent()->uniqueTypeWithNewShape(

RC->getType(), LRN->getInput().dims());

auto *newC = F->getParent()->createConstant(ty, RC->getName());

newC->getPayloadMutable().copyRawFrom(&RC->getPayload());

rhs = newC->getOutput();

}

if (RQ) {

auto ty = F->getParent()->uniqueTypeWithNewShape(

RQ->getResult().getType(), LRN->getInput().dims());

rhs = F->createQuantize(RQ->getName(), rhs, ty);

}

if (RS) {

auto ty = F->getParent()->uniqueTypeWithNewShape(

RS->getResult().getType(), LRN->getInput().dims());

rhs = F->createSplat(RS->getName(), ty, RS->getValue());

}

// Reshape RHS to match LHS.

NodeValue rhs = F->createSplat(nv.getNode()->getName(), nv, LRN->getInput().dims());

jfix71 · 2021-07-16T21:52:38Z

lib/Optimizer/GraphOptimizer/GraphOptimizer.cpp

+        // RHS must be either a Reshape or a Constant (+ Quantize) or Splat.
+        auto *RQ = dyn_cast<QuantizeNode>(nv);
+        auto *RC = dyn_cast<Constant>(RQ ? RQ->getInput() : nv);
+        auto *RS = dyn_cast<SplatNode>(nv);
+        if (!RRN && !RC && !RS) {
+          continue;
+        }


Combined with the below suggestion of not relying on specific nodes -- can we just check if this is a constant chain of operations and fold if so? I.e. we could make isConstantOperation from ConstantFolding.cpp usable outside of there and call it here, e.g.

Suggested change

// RHS must be either a Reshape or a Constant (+ Quantize) or Splat.

auto *RQ = dyn_cast<QuantizeNode>(nv);

auto *RC = dyn_cast<Constant>(RQ ? RQ->getInput() : nv);

auto *RS = dyn_cast<SplatNode>(nv);

if (!RRN && !RC && !RS) {

continue;

}

// RHS must be a constant chain if it's not a reshape, to allow for the reshape to get folded into the chain later on.

if (!RRN || !isConstantOperation(nv)) {

continue;

}

(We'd need to do something like assume Interpreter is the constant folding backend, but I think that's generally the case anyway...)

If we use isConstantOperation, we'll change the algorithm from O(N) to something like O(N^2). But the optimization should indeed benefit from it. Is that ok?

stale · 2021-08-21T04:46:46Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

aturetsk · 2021-08-23T19:39:23Z

@jfix71, can you comment on the following before I proceed with updating the changes?

If we use isConstantOperation, we'll change the algorithm from O(N) to something like O(N^2). But the optimization should indeed benefit from it. Is that ok?

stale · 2022-03-02T10:08:48Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

stale · 2022-04-16T16:30:47Z

This PR has been automatically closed due to being stale for 15 days. Thank you for your contributions and feel free to reopen it in case of further progress.

facebook-github-bot added the CLA Signed label Jun 10, 2021

[GraphOptimizer] Extend Reshape sinking pass for binary eltwise ops.

bbc3eab

aturetsk force-pushed the cadence/turetski/reshape-sinking branch from 22139d9 to bbc3eab Compare June 10, 2021 19:53

stale bot added the stale_will_be_closed label Jul 1, 2021

stale bot removed the stale_will_be_closed label Jul 12, 2021

jfix71 reviewed Jul 16, 2021

View reviewed changes

stale bot added the stale_will_be_closed label Aug 21, 2021

stale bot removed the stale_will_be_closed label Aug 23, 2021

stale bot added the stale_will_be_closed label Mar 2, 2022

stale bot closed this Apr 16, 2022

		.setDocstring("Selects indices of the true elements in Cond");
		// clang-format on

[GraphOptimizer] Extend Reshape sinking pass for binary eltwise ops #5715

[GraphOptimizer] Extend Reshape sinking pass for binary eltwise ops #5715

Uh oh!

Conversation

aturetsk commented Jun 10, 2021

Uh oh!

aturetsk commented Jun 10, 2021

Uh oh!

stale bot commented Jul 1, 2021

Uh oh!

aturetsk commented Jul 1, 2021

Uh oh!

aturetsk commented Jul 12, 2021

Uh oh!

jfix71 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Aug 21, 2021

Uh oh!

aturetsk commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Mar 2, 2022

Uh oh!

stale bot commented Apr 16, 2022

Uh oh!

Uh oh!

aturetsk commented Aug 23, 2021 •

edited

Loading