fix final problems caused by the rebase to c8ac18d #17217

marcrasi · 2018-06-14T20:45:00Z

These fixes make everything compile, and all the test pass.

I'll add github comments in the PR explaining the reasons for some of the changes.

(Note that this PR is into a tensorflow-rebase branch, not into tensorflow, so I'll need to do one final step of moving tensorflow-rebase into tensorflow after I land this PR).

marcrasi · 2018-06-14T20:46:25Z

lib/SILOptimizer/Mandatory/MandatoryInlining.cpp

I moved these checks into the shouldInlinePredicate function, so that we can have different checks for TF and for non-TF inlining.

This is probably not the most correct solution, but it makes all the tests pass.

If TF mandatory inlining needs to inline non-transaprent functions, please update the SIL linker in lib/SIL/Linker.cpp and lib/SILOptimizer/UtilityPasses/Link.cpp.

@slavapestov we should probably do that in a follow-on patch. As of yet, we don't have an attribute to guide that inlining decision. It is guided by structural concerns, so it may be more difficult for us to do. We may have to introduce an attribute for this concept (probably should have long ago).

@marcrasi can you please file a jira to track this for resolution after the merge?

Done: https://bugs.swift.org/browse/SR-8015

I'll also add a todo in the code here referencing the issue.

dan-zheng · 2018-06-14T20:47:03Z

utils/update-checkout-config.json

Minor: would you mind reordering these to be in the original order?

marcrasi · 2018-06-14T20:47:25Z

utils/update-checkout-config.json

I pointed these at my forks of the repos, because I don't have commit access to the original repos yet. I'll point these back to the original repos before I merge this PR.

marcrasi · 2018-06-14T20:48:42Z

test/TensorFlow/integration.swift

I am guessing that these differences are caused by the switch to guaranteed calling, but could someone confirm that this is correct?

Can you provide the full SIL code?

here's the SIL for the TFPartition Host Result for this function: https://gist.github.com/marcrasi/b2fdaeb542b9b9d307bb49efe14e1406

I also included the TFPartition Host Result for the function that I changed in retain_release.swift, in case you want to look at that.

Looks good. Thanks!

marcrasi · 2018-06-14T20:49:01Z

test/TensorFlow/retain_release.swift

Yes this should be fine as it's add and removing the same # retain/release insts.

marcrasi · 2018-06-14T20:50:05Z

lib/AST/ASTPrinter.cpp

I cherry-picked this change from master, because it makes a failing test pass.

marcrasi · 2018-06-14T20:50:28Z

lib/SIL/SILPrinter.cpp

This is just part of the cherry-picked change that I mention above.

rxwei · 2018-06-14T21:02:44Z

lib/SILOptimizer/Mandatory/TFPartition.cpp

I think "noNestedConflict" is what's in the function signature.

done (all of them)

rxwei · 2018-06-14T21:02:48Z

lib/SILOptimizer/Mandatory/TFPartition.cpp

"noNestedConflict"

rxwei · 2018-06-14T21:02:52Z

lib/SILOptimizer/Mandatory/TFPartition.cpp

"noNestedConflict"

rxwei · 2018-06-14T21:02:58Z

lib/SILOptimizer/Mandatory/TFPartition.cpp

"noNestedConflict"

mhong

Changes in TFUtilities.cpp and TFPartition.cpp look good.

There's one unit test change that I commented out, and can look at more when you provide the full SIL code.

mhong · 2018-06-14T21:59:32Z

test/TensorFlow/integration.swift

Can you provide the full SIL code?

mhong · 2018-06-14T22:00:11Z

test/TensorFlow/retain_release.swift

a heads-up that i've seen piper behave differently in this regard. one option is to replace with {{.*}}

FileCheck doesn't require that a CHECK matches the entire line, so it should already work no matter what stuff appears in front.

You are right.

mhong · 2018-06-14T22:01:19Z

test/TensorFlow/retain_release.swift

Yes this should be fine as it's add and removing the same # retain/release insts.

lattner

LGTM with minor formatting changes, thank you!

lattner · 2018-06-15T03:18:15Z

lib/AST/ASTPrinter.cpp

lattner · 2018-06-15T03:20:26Z

lib/SILOptimizer/Mandatory/MandatoryInlining.cpp

@slavapestov we should probably do that in a follow-on patch. As of yet, we don't have an attribute to guide that inlining decision. It is guided by structural concerns, so it may be more difficult for us to do. We may have to introduce an attribute for this concept (probably should have long ago).

@marcrasi can you please file a jira to track this for resolution after the merge?

lattner · 2018-06-15T03:21:00Z

lib/SILOptimizer/Mandatory/TFDeabstraction.cpp

Plz wrap after the && to fit in 80 cols.

lattner · 2018-06-15T03:21:42Z

lib/Sema/CSApply.cpp

Plz indent by 2.

lattner · 2018-06-15T03:22:13Z

stdlib/public/TensorFlow/CompilerRuntime.swift

This " Only defined when usesTFEagerAPI == false." comment should be removed.

Includes cherry-pick of d093879 to fix a test.

Includes cherry-pick of d093879 to fix a test. These fixes make everything compile, and all the test pass.

marcrasi added the tensorflow This is for "tensorflow" branch PRs. label Jun 14, 2018

marcrasi requested review from dan-zheng, lattner, mhong and rxwei June 14, 2018 20:45

marcrasi commented Jun 14, 2018

View reviewed changes

dan-zheng reviewed Jun 14, 2018

View reviewed changes

marcrasi commented Jun 14, 2018

View reviewed changes

lib/SIL/SILPrinter.cpp Outdated

Copy link

Author

marcrasi Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just part of the cherry-picked change that I mention above.

rxwei approved these changes Jun 14, 2018

View reviewed changes

mhong reviewed Jun 14, 2018

View reviewed changes

mhong approved these changes Jun 14, 2018

View reviewed changes

lattner approved these changes Jun 15, 2018

View reviewed changes

marcrasi force-pushed the fix-rebase-problems branch 3 times, most recently from 0fef0df to 9d6175b Compare June 18, 2018 19:07

fix problems caused by the rebase to c8ac18d

70b3804

Includes cherry-pick of d093879 to fix a test.

marcrasi force-pushed the fix-rebase-problems branch from 9d6175b to 70b3804 Compare June 18, 2018 19:43

marcrasi merged commit 13f448c into swiftlang:tensorflow-rebase Jun 18, 2018

marcrasi added a commit that referenced this pull request Jun 22, 2018

fix problems caused by the rebase to c8ac18d (#17217)

aecab01

Includes cherry-pick of d093879 to fix a test. These fixes make everything compile, and all the test pass.

marcrasi added a commit that referenced this pull request Jun 28, 2018

fix problems caused by the rebase to c8ac18d (#17217)

3e6ed38

Includes cherry-pick of d093879 to fix a test. These fixes make everything compile, and all the test pass.

marcrasi mannequin mentioned this pull request Apr 25, 2022

[SR-8015] handle TF inlining properly #50548

Closed

fix final problems caused by the rebase to c8ac18d #17217

fix final problems caused by the rebase to c8ac18d #17217

Uh oh!

Conversation

marcrasi commented Jun 14, 2018

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!

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!

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!

mhong 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!

lattner 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development