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

add TouchInst for buffers that don't need to be initialized #4000

Closed
wants to merge 1 commit into from

Conversation

@nickgg
Copy link
Contributor

nickgg commented Jan 13, 2020

Summary: Per #3903 when IRGening ConcatNode we insert a SplatInst so the output buffer is initialized. In that case since we overwrite the entire buffer we don't need that splat and it's wasted work. This PR solves the problem by doing nothing, or rather by adding a new instruction TouchInst which does nothing except mark its output as initialized. I've only updated the case of Concat, but there may be other situations we use splat as an initializer for memory that is overwritten.

Documentation: N/A

Test Plan: Ran tests in debug and release. This code is covered by the Concat operator tests for backends that use IR - Interpreter, CPU and OpenCL (Habana and NNPI use Nodes instead).

Fixes #3903.

@nickgg nickgg requested a review from jfix71 Jan 13, 2020
Copy link

facebook-github-bot left a comment

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

@nickgg

This comment has been minimized.

Copy link
Contributor Author

nickgg commented Jan 13, 2020

This instruction must be implemented on all backends that use Glow IR (although it does nothing). CC: @mciprian13, @et-nivard.

@jfix71
jfix71 approved these changes Jan 13, 2020
Copy link
Contributor

jfix71 left a comment

LGTM, two nits. Glad this was so simple!

@@ -621,6 +621,11 @@ int main(int argc, char **argv) {
.autoVerify(VerifyKind::NoVerify)
.autoIRGen();

BB.newInstr("Touch")
.addOperand("Dest", OperandKind::Out)
.dataParallel()

This comment has been minimized.

Copy link
@jfix71

jfix71 Jan 13, 2020

Contributor

We can skip this, right? Seems a little misleading/weird to mark essentially a nop as data parallel.

This comment has been minimized.

Copy link
@nickgg

nickgg Jan 13, 2020

Author Contributor

We cannot skip this. I'm not 100% sure why but Instructions that only have an output (e.g. splat) must be data parallel or they don't work.

lib/IR/IRGen.cpp Outdated Show resolved Hide resolved
@nickgg nickgg force-pushed the nickgg:touchinst branch from d74e059 to b87419a Jan 13, 2020
Copy link

facebook-github-bot left a comment

@nickgg is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jan 14, 2020

@nickgg merged this pull request in bc8a675.

UncleGene added a commit to UncleGene/glow that referenced this pull request Jan 15, 2020
…4000)

Summary:
Per pytorch#3903 when IRGening ConcatNode we insert a `SplatInst` so the output buffer is initialized. In that case since we overwrite the entire buffer we don't need that splat and it's wasted work. This PR solves the problem by doing nothing, or rather by adding a new instruction `TouchInst` which does nothing except mark its output as initialized. I've only updated the case of Concat, but there may be other situations we use `splat` as an initializer for memory that is overwritten.

Documentation: N/A
Pull Request resolved: pytorch#4000

Test Plan:
Ran tests in debug and release. This code is covered by the Concat operator tests for backends that use IR - Interpreter, CPU and OpenCL (Habana and NNPI use Nodes instead).

Fixes pytorch#3903.

Differential Revision: D19376756

Pulled By: nickgg

fbshipit-source-id: 2eccc64f3501090c5919b8162502586877eaf716
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.