GPU fusion code cleanup: Extract GpuInstructionFusion::IsFusible into gpu_fusible.cc #25842

sana-damani · 2019-02-18T14:47:18Z

No description provided.

… gpu_fusible.cc Tested using //tensorflow/compiler/xla/service/gpu/tests:gpu_fusion_test

googlebot · 2019-02-18T14:47:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

Googlers can find more info about SignCLA and this PR by following this link.

sana-damani · 2019-02-18T15:00:55Z

I have signed the CLA

rthadur · 2019-02-19T18:03:47Z

@sana-damani please sign CLA

sana-damani · 2019-02-19T18:20:50Z

https://cla.developers.google.com/clas shows that I signed it on Feb 18. Sana

…

On Tue, Feb 19, 2019 at 1:13 PM rthadur ***@***.***> wrote: @sana-damani <https://github.com/sana-damani> please sign CLA — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25842 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUshHWrvxoAgR4Ocklk6SAAIi7rssJxDks5vPD6ugaJpZM4bBBWw> .

Tested using //tensorflow/compiler/xla/service/gpu/tests:gpu_fusion_test and //tensorflow/python/compiler

sanjoy · 2019-02-21T00:46:33Z

@rthadur Any idea what we're waiting on here?

sana-damani · 2019-02-21T15:53:13Z

@rthadur Any idea what we're waiting on here?

Looks like it needs another review before it can be merged. This is likely because I committed a new change before the merge was done.

sanjoy · 2019-02-21T17:06:48Z

@rthadur I still see the "Need a CLA for one or more commit authors" line even though @sana-damani says they've signed the CLA.

rthadur · 2019-02-21T17:59:26Z

@rthadur I still see the "Need a CLA for one or more commit authors" line even though @sana-damani says they've signed the CLA.

@sanjoy i believe there were multiple people who contributed to this PR , @sana-damani do you know if there is one more contributor for this PR ?

sana-damani · 2019-02-21T18:06:01Z

I was the only contributor.

…

On Thu, Feb 21, 2019, 1:04 PM rthadur ***@***.***> wrote: @rthadur <https://github.com/rthadur> I still see the "Need a CLA for one or more commit authors" line even though @sana-damani <https://github.com/sana-damani> says they've signed the CLA. @sanjoy <https://github.com/sanjoy> i believe there were multiple people who contributed to this PR , @sana-damani <https://github.com/sana-damani> do you know if there is one more contributor for this PR ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25842 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUshHYW7E5bKGOe45Cr_G-fKMgzsolorks5vPt-hgaJpZM4bBBWw> .

yifeif · 2019-02-21T18:36:19Z

Hi @sana-damani, did you use the same username and email address for your commits? I see "sdamani" as the username on the commits.

sana-damani · 2019-02-21T20:09:51Z

Thanks for pointing that out! I'll try and fix the commit author.

…

On Thu, Feb 21, 2019 at 1:41 PM Yifei Feng ***@***.***> wrote: Hi @sana-damani <https://github.com/sana-damani>, did you use the same username and email address for your commits? I see "sdamani" as the username on the commits. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25842 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUshHcShGdnloKu4Ny9_7HwDAjaKgSq4ks5vPuhPgaJpZM4bBBWw> .

sana-damani · 2019-02-21T20:25:30Z

I have signed the CLA with the updated username

googlebot · 2019-02-21T21:15:25Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

sana-damani · 2019-02-26T19:52:25Z

Is the Windows Bazel GPU build failure a real failure? If so, is there a way that I can reproduce and debug this failure?

sanjoy · 2019-02-27T04:53:42Z

I don't think the windows failure is related.

thomasjoerg · 2019-02-27T07:19:00Z

tensorflow/compiler/xla/service/gpu/gpu_fusible.cc

+bool IsInputFusible(const HloInstruction& instr) {
+  // Input fusion only handles non-elemental reduction and scatter operations.
+  return IsInputFusibleReduction(instr) ||
+         instr.opcode() == HloOpcode::kScatter;


The function works for unfused Scatter ops but not for existing Scatter input fusions. You want to add something like this.

(instr.opcode() == HloOpcode::kFusion && instr.fusion_kind() == HloInstruction::FusionKind::kInput && instr.fused_expression_root()->opcode() == HloOpcode::kScatter)

thomasjoerg · 2019-02-27T09:15:00Z

tensorflow/compiler/xla/service/gpu/gpu_fusible.cc

@@ -150,8 +150,7 @@ bool IsLoopFusible(const HloInstruction& instr) {
         instr.opcode() == HloOpcode::kConcatenate ||
         instr.opcode() == HloOpcode::kDynamicSlice ||
         instr.opcode() == HloOpcode::kDynamicUpdateSlice ||
-         (instr.opcode() == HloOpcode::kFusion &&
-          instr.fusion_kind() == HloInstruction::FusionKind::kLoop) ||
+         instr.opcode() == HloOpcode::kFusion ||


Existing input fusions are not loop-fusible, hence we should return false as we did before. Please update IsInputFusible as suggested in my other comment instead.

What about output and custom fusion nodes? Should they also be disallowed from loop fusion?

Yes, that's a good point. We want to check for FusionKind::kLoop here.

googlebot · 2019-03-01T21:09:53Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-01T21:22:38Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

thomasjoerg · 2019-03-04T07:29:51Z

tensorflow/compiler/xla/service/gpu/gpu_fusible.h

@@ -46,6 +55,10 @@ bool IsReduceInputFusion(const HloInstruction& instr);
 // is either an unfused reduction-to-vector op or a reduce input fusion.
 bool IsInputFusibleReduction(const HloInstruction& instr);

+// Whether `instr` is fusible as root of a reduce input fusions, i.e. `instr`


Please replace "reduce" with "scatter".

googlebot · 2019-03-04T19:21:18Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-04T19:33:29Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

PiperOrigin-RevId: 236829871

sdamani added 4 commits February 16, 2019 13:44

GPU fusion code cleanup: Extract GpuInstructionFusion::IsFusible into…

abb86cd

… gpu_fusible.cc Tested using //tensorflow/compiler/xla/service/gpu/tests:gpu_fusion_test

Remove extra whitespace

432522e

Fix IsFusible comments

106e549

Fix formatting issues in IsFusible

2184ace

googlebot added the cla: no label Feb 18, 2019

thomasjoerg previously approved these changes Feb 19, 2019

View reviewed changes

rthadur self-assigned this Feb 19, 2019

Refactor IsFusible() into IsLoopFusible() and IsInputFusible().

2ab789a

Tested using //tensorflow/compiler/xla/service/gpu/tests:gpu_fusion_test and //tensorflow/python/compiler

sana-damani dismissed thomasjoerg’s stale review via 2ab789a February 20, 2019 16:24

sanjoy previously approved these changes Feb 21, 2019

View reviewed changes

rthadur added cla: yes and removed cla: no labels Feb 21, 2019

rthadur added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process size:M CL Change Size: Medium labels Feb 21, 2019

rthadur added this to Assigned Reviewer in PR Queue via automation Feb 21, 2019

kokoro-team removed the kokoro:force-run Tests on submitted change label Feb 21, 2019

thomasjoerg reviewed Feb 27, 2019

View reviewed changes

sdamani added 2 commits March 1, 2019 10:37

Add IsInputFusibleScatter()

81df928

Only fuse loop fusion computations

194c671

sana-damani dismissed sanjoy’s stale review via 194c671 March 1, 2019 21:09

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Mar 1, 2019

googlebot added cla: no and removed cla: yes labels Mar 1, 2019

sanjoy added cla: yes and removed cla: no labels Mar 1, 2019

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 4, 2019

thomasjoerg previously approved these changes Mar 4, 2019

View reviewed changes

Change comment to reflect scatter op

aea1596

sana-damani dismissed thomasjoerg’s stale review via aea1596 March 4, 2019 19:21

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Mar 4, 2019

googlebot added cla: no and removed cla: yes labels Mar 4, 2019

sanjoy added cla: yes and removed cla: no labels Mar 4, 2019

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 5, 2019

thomasjoerg approved these changes Mar 5, 2019

View reviewed changes

pull bot pushed a commit to Rachelmorrell/tensorflow that referenced this pull request Mar 5, 2019

Merge pull request tensorflow#25842 from sana-damani:master

3a85208

PiperOrigin-RevId: 236829871

tensorflow-copybara merged commit aea1596 into tensorflow:master Mar 5, 2019

PR Queue automation moved this from Approved by Reviewer to Merged Mar 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPU fusion code cleanup: Extract GpuInstructionFusion::IsFusible into gpu_fusible.cc #25842

GPU fusion code cleanup: Extract GpuInstructionFusion::IsFusible into gpu_fusible.cc #25842

sana-damani commented Feb 18, 2019

googlebot commented Feb 18, 2019

sana-damani commented Feb 18, 2019

rthadur commented Feb 19, 2019

sana-damani commented Feb 19, 2019 via email

sanjoy commented Feb 21, 2019

sana-damani commented Feb 21, 2019

sanjoy commented Feb 21, 2019

rthadur commented Feb 21, 2019

sana-damani commented Feb 21, 2019 via email

yifeif commented Feb 21, 2019

sana-damani commented Feb 21, 2019 via email

sana-damani commented Feb 21, 2019

googlebot commented Feb 21, 2019

sana-damani commented Feb 26, 2019

sanjoy commented Feb 27, 2019

thomasjoerg Feb 27, 2019 •

edited

thomasjoerg Feb 27, 2019

sana-damani Feb 27, 2019

thomasjoerg Feb 28, 2019

googlebot commented Mar 1, 2019

googlebot commented Mar 1, 2019

thomasjoerg Mar 4, 2019

googlebot commented Mar 4, 2019

googlebot commented Mar 4, 2019

GPU fusion code cleanup: Extract GpuInstructionFusion::IsFusible into gpu_fusible.cc #25842

GPU fusion code cleanup: Extract GpuInstructionFusion::IsFusible into gpu_fusible.cc #25842

Conversation

sana-damani commented Feb 18, 2019

googlebot commented Feb 18, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

sana-damani commented Feb 18, 2019

rthadur commented Feb 19, 2019

sana-damani commented Feb 19, 2019 via email

sanjoy commented Feb 21, 2019

sana-damani commented Feb 21, 2019

sanjoy commented Feb 21, 2019

rthadur commented Feb 21, 2019

sana-damani commented Feb 21, 2019 via email

yifeif commented Feb 21, 2019

sana-damani commented Feb 21, 2019 via email

sana-damani commented Feb 21, 2019

googlebot commented Feb 21, 2019

sana-damani commented Feb 26, 2019

sanjoy commented Feb 27, 2019

thomasjoerg Feb 27, 2019 • edited

Choose a reason for hiding this comment

thomasjoerg Feb 27, 2019

Choose a reason for hiding this comment

sana-damani Feb 27, 2019

Choose a reason for hiding this comment

thomasjoerg Feb 28, 2019

Choose a reason for hiding this comment

googlebot commented Mar 1, 2019

googlebot commented Mar 1, 2019

thomasjoerg Mar 4, 2019

Choose a reason for hiding this comment

googlebot commented Mar 4, 2019

googlebot commented Mar 4, 2019

thomasjoerg Feb 27, 2019 •

edited