Support Backend Specific Verifiers #3469

shajrawi · 2019-08-28T21:16:07Z

Summary:
Adds support for backend-specific verifiers to Glow.
I removed the asserts in the backend specific code when executing Nodes with components that depend on the backend for validity and added verifiers to take their place.
There are two backend-specific verifiers: one that works on Function and one that takes in IRFunction - this allows verification at different stages of the Glow pipeline.
I also added two error types: one that indicates a verification error at initial IR generation (COMPILE_UNSUPPORTED_IR_AFTER_GENERATE) and one that indicates the faulty code appeared after IR optimizations (COMPILE_UNSUPPORTED_IR_AFTER_OPTIMIZE).
I also refactored the code, such that Graph Optimizer does not check that all nodes are supported directly, that should not be its job, it calls the verifier instead. The base method for the Function verifier does said check.

Fixes #3450

Test Plan:
ninja test

lib/Backends/OpenCL/OpenCL.cpp

lib/Backends/Interpreter/Interpreter.cpp

jfix71

Looks pretty good to me! Just a couple minor nits, and agree would be nice to reduce the boilerplate stuff.

include/glow/Backend/Backend.h

lib/Backends/Interpreter/Interpreter.cpp

opti-mix · 2019-08-28T22:56:44Z

@shajrawi Overall, looks pretty good! Thanks for adding this feature!

ayermolo · 2019-08-28T23:05:41Z

Somewhat tangential, but right now isSupported is used for two things.

Verify that this node is legal for specific backend.
During quantization to determine if node can be converted to quantized.

To me it seems that it should be two different functions to answer those two questions.

jfix71 · 2019-08-28T23:15:44Z

@shajrawi Can you also update docs/Backends.md?

jfix71 · 2019-08-28T23:20:57Z

@ayermolo The check for if a node is supported as quantized is a subset of isOpSupported(). If we moved them to two separate functions we'd likely duplicate a lot of code. Though I believe we'd also be able to pass in a Node directly into isOpSupported() instead of NodeInfo, allowing for checking members and actual input/output Nodes instead of just input/output types.

ayermolo · 2019-08-29T00:34:21Z

Well isOpSupported can always call canQuantize (or whatever the other function name ends up). This way BE can control which nodes get quantized type, and also do additional checks on generic nodes to make sure they conform to BE requirements.

Although I guess with ability to override verify the BE will be able to do it anyway after the check for isOpSupported.

shajrawi · 2019-08-29T04:07:02Z

@jfix71 I resolved the minor nits and updated docs/Backends.md
@opti-mix , @nickgg I created templated solution for most grievous repetition, as a bonus my solution checks for fusion (OpenCL and Interpreter backends) and NHWC layout (Interpreter backend) for all ALL Node(s) and Instruction(s) that contain layout / fusion fields without specifying them directly. making it future proof :)

opti-mix

Thanks for introducing those helpers! Looks better now.

lib/Backend/BackendUtils.cpp

opti-mix · 2019-08-29T14:16:10Z

include/glow/Backend/BackendUtils.h

@@ -173,6 +173,46 @@ bool isOutput(const Placeholder *PH, const IRFunction &F);
 /// by the current function.
 bool isInput(const Placeholder *PH, const IRFunction &F);

+/// Checks if typename C contains getFusedActivation() method


May be add "Use template meta-programming to check if ..."

Using this approach is very clever! May be too clever ;-)

lib/Backends/Interpreter/Interpreter.cpp

include/glow/Backend/Backend.h

opti-mix · 2019-08-29T14:34:55Z

lib/Optimizer/GraphOptimizer/GraphOptimizer.cpp

@@ -3017,5 +2997,11 @@ llvm::Error glow::optimizeFunction(Function *F, const Backend &B,
    ::glow::optimize(F, cctx, B);
  }

-  return checkAllNodesSupported(*F, B);
+  if (!B.verify(*F)) {


This deserves some comment IMHO. If I understand correctly, our strategy here is that we start using backend specific verification only after transformPostLowering, i.e. only after some backend-specific nodes were introduced eventually. Is it a correct understanding? Please explain it in the comment then.

But I've got another related question here:

We may call some graph optimizations multiple times. And we may call verify after each pass IIRC (see the pass manager details). Which verifier is currently called after each pass though and which one should be called? The backend-independent one or the backend-specific one? Or should it depend on the stage of compilation (e.g. call the backend-independent one before transformPostLowering and the backend-specific one after transformPostLowering)? WDYT?

Excellent observation! I changed this behavior and made it explicit in a separate commit that I added to this PR!

We should always do backend independent verifications of course, because we don't duplicate work, however, we should also do backend specific verifications form the moment we have the function in a lowered state.

The pass manager now checks the function state and adds extra verifications appropriately.

I'm not sure I can follow the logic of this new commit. Some of the optimizations are essentially backend-independent. But it seems like you demand now to always pass a Backend to the pass manager, even if the pass manager does not use it at the given stage of compilation. As a result you had to touch a bunch of places and pass a fake Backend as a parameter now. This seems odd to me. And if this backend is always used (may be I overlooked something), which concrete Backend would you use before introducing target-specific nodes? Looks like you'd need a default Glow backend, which just captures the default Glow requirements in this case.

I updated the the follow-up commit to avoid that. we now pass a pointer instead of a reference, with a default value of nullptr, so we no longer need to pollute the code base with the fake backend. I think we only need to pass the backend when we are doing backend specific optimizations. WDYT?

Yess, makes sense.

include/glow/Optimizer/GraphOptimizer/PassManager.h

include/glow/Backend/BackendUtils.h

lib/Backends/Interpreter/Interpreter.cpp

include/glow/Optimizer/GraphOptimizer/GraphOptimizer.h

opti-mix · 2019-08-29T17:50:50Z

lib/Optimizer/GraphOptimizer/GraphOptimizer.cpp

@@ -3017,5 +2997,11 @@ llvm::Error glow::optimizeFunction(Function *F, const Backend &B,
    ::glow::optimize(F, cctx, B);
  }

-  return checkAllNodesSupported(*F, B);
+  if (!B.verify(*F)) {


I'm not sure I can follow the logic of this new commit. Some of the optimizations are essentially backend-independent. But it seems like you demand now to always pass a Backend to the pass manager, even if the pass manager does not use it at the given stage of compilation. As a result you had to touch a bunch of places and pass a fake Backend as a parameter now. This seems odd to me. And if this backend is always used (may be I overlooked something), which concrete Backend would you use before introducing target-specific nodes? Looks like you'd need a default Glow backend, which just captures the default Glow requirements in this case.

lib/Optimizer/GraphOptimizer/PassManager.cpp

jfix71

Looks pretty good to me, just a series of nits.

include/glow/Backend/BackendUtils.h

lib/Optimizer/GraphOptimizer/GraphOptimizer.cpp

lib/Optimizer/GraphOptimizer/PassManager.cpp

lib/Optimizer/GraphOptimizer/GraphOptimizer.cpp

shajrawi · 2019-08-29T21:02:56Z

Amended the PR to address the nits :)

opti-mix

Looks pretty-good now!

I'm still thinking about the condition Backend verifications is always a super-set of F->verify(). Is it really the case under all circumstances?

lib/Optimizer/GraphOptimizer/GraphOptimizer.cpp

lib/Optimizer/GraphOptimizer/PassManager.cpp

include/glow/Backend/Backend.h

opti-mix · 2019-08-29T21:04:47Z

lib/Optimizer/GraphOptimizer/GraphOptimizer.cpp

@@ -3017,5 +2997,11 @@ llvm::Error glow::optimizeFunction(Function *F, const Backend &B,
    ::glow::optimize(F, cctx, B);
  }

-  return checkAllNodesSupported(*F, B);
+  if (!B.verify(*F)) {


Yess, makes sense.

ayermolo · 2019-08-29T18:59:19Z

lib/Backend/Backend.cpp

+  return checkAllNodesSupported(F);
+}
+
+bool Backend::verify(const IRFunction &IR) const { return true; }


Maybe it's better to make this pure virtual to force BE to deal with it, versus thinking it does something when it doesn't?

I agree. Would like to turn them into pure virtual, but, I don't want to break any backends when doing this commit + I will intend to create a beginner / good first issue for someone to do write a verifier for the Habana backend.

shajrawi · 2019-08-29T22:14:14Z

@opti-mix Fixed the nits + I made an explicit call to the target independent verifier to make sure.

include/glow/Backend/Backend.h

lib/Optimizer/GraphOptimizer/PassManager.cpp

shajrawi · 2019-08-29T23:28:23Z

Fixed the nits / updated the comments. and all the tests pass.

opti-mix

Looks good! Great job!

Just address the final minor grammar nits (ideally, without introducing new ones ;-) and ship it!
🚀

docs/Backends.md

lib/Backend/BackendUtils.cpp

include/glow/Backend/BackendUtils.h

lib/Backends/Interpreter/Interpreter.cpp

lib/Backends/OpenCL/OpenCL.cpp

lib/Optimizer/GraphOptimizer/PassManager.cpp

lib/Backends/OpenCL/OpenCL.cpp

opti-mix · 2019-08-30T04:21:39Z

👏 🎆

facebook-github-bot

@shajrawi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…pass manager

facebook-github-bot

@shajrawi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-08-30T07:08:52Z

@shajrawi merged this pull request in 46dcb19.

facebook-github-bot added the CLA Signed label Aug 28, 2019

shajrawi requested review from nickgg, jfix71, jackm321 and opti-mix August 28, 2019 21:18

rdzhabarov assigned nickgg and opti-mix Aug 28, 2019

nickgg reviewed Aug 28, 2019

View reviewed changes

lib/Backends/OpenCL/OpenCL.cpp Outdated Show resolved Hide resolved

lib/Backends/Interpreter/Interpreter.cpp Show resolved Hide resolved

shajrawi force-pushed the backend_verify branch 2 times, most recently from b1f72f3 to daa3eb3 Compare August 28, 2019 22:28

jfix71 reviewed Aug 28, 2019

View reviewed changes

include/glow/Backend/Backend.h Outdated Show resolved Hide resolved

lib/Backends/Interpreter/Interpreter.cpp Outdated Show resolved Hide resolved

shajrawi force-pushed the backend_verify branch from daa3eb3 to 2915513 Compare August 29, 2019 04:00

shajrawi force-pushed the backend_verify branch 2 times, most recently from 311bcb4 to 36f193b Compare August 29, 2019 04:52

opti-mix reviewed Aug 29, 2019

View reviewed changes

shajrawi force-pushed the backend_verify branch 2 times, most recently from 8883258 to 596b7a2 Compare August 29, 2019 17:44

opti-mix reviewed Aug 29, 2019

View reviewed changes

Support Backend Specific Verifiers

fda77b0

shajrawi force-pushed the backend_verify branch from 596b7a2 to c4aac21 Compare August 29, 2019 18:21

jfix71 reviewed Aug 29, 2019

View reviewed changes

shajrawi force-pushed the backend_verify branch from c4aac21 to 07007e1 Compare August 29, 2019 21:01

opti-mix reviewed Aug 29, 2019

View reviewed changes

ayermolo reviewed Aug 29, 2019

View reviewed changes

shajrawi force-pushed the backend_verify branch from 07007e1 to 8b811ec Compare August 29, 2019 22:13

shajrawi force-pushed the backend_verify branch 2 times, most recently from 927b9ad to 634115a Compare August 29, 2019 22:55

opti-mix reviewed Aug 29, 2019

View reviewed changes

shajrawi force-pushed the backend_verify branch from 634115a to 3d9c0c9 Compare August 29, 2019 23:10

opti-mix approved these changes Aug 29, 2019

View reviewed changes

shajrawi force-pushed the backend_verify branch from 3d9c0c9 to 833cd5c Compare August 30, 2019 04:15

opti-mix reviewed Aug 30, 2019

View reviewed changes

lib/Backends/OpenCL/OpenCL.cpp Outdated Show resolved Hide resolved

shajrawi force-pushed the backend_verify branch from 833cd5c to 2803a83 Compare August 30, 2019 04:19

facebook-github-bot reviewed Aug 30, 2019

View reviewed changes

Add support for doing backend specific verifications to the function …

b2cd334

…pass manager

shajrawi force-pushed the backend_verify branch from 2803a83 to b2cd334 Compare August 30, 2019 05:30

facebook-github-bot reviewed Aug 30, 2019

View reviewed changes

facebook-github-bot closed this in 46dcb19 Aug 30, 2019

facebook-github-bot added the Merged label Aug 30, 2019

shajrawi deleted the backend_verify branch August 30, 2019 16:38

shajrawi mentioned this pull request Aug 30, 2019

[habana] Implement backend specific verifications #3480

Open

Support Backend Specific Verifiers #3469

Support Backend Specific Verifiers #3469

Uh oh!

Conversation

shajrawi commented Aug 28, 2019

Uh oh!

Uh oh!

Uh oh!

jfix71 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

opti-mix commented Aug 28, 2019

Uh oh!

ayermolo commented Aug 28, 2019

Uh oh!

jfix71 commented Aug 28, 2019

Uh oh!

jfix71 commented Aug 28, 2019

Uh oh!

ayermolo commented Aug 29, 2019

Uh oh!

shajrawi commented Aug 29, 2019

Uh oh!

opti-mix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jfix71 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shajrawi commented Aug 29, 2019

Uh oh!

opti-mix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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