Skip to content

Conversation

t-vi
Copy link
Collaborator

@t-vi t-vi commented Sep 29, 2020

This might be an alternative to reverting #45396 .
The obvious rough edge is that I'm not really seeing the work group limits that TensorExpr produces.

@t-vi t-vi requested a review from apaszke as a code owner September 29, 2020 15:49
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 29, 2020
@dr-ci
Copy link

dr-ci bot commented Sep 29, 2020

💊 CI failures summary and remediations

As of commit 71bf92a (more details on the Dr. CI page):



XLA failure

Job pytorch_xla_linux_bionic_py3_6_clang9_build is failing. Please create an issue with title prefixed by [PT_BREAK] in pytorch/xla and link to to this PR. If you have questions, please reach out to @ailzhang / @dlibenzi / @JackCaoG.


🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

Since your merge base is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.


ci.pytorch.org: 1 failed


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 20 times.

@t-vi
Copy link
Collaborator Author

t-vi commented Sep 29, 2020

@t-vi t-vi force-pushed the ROCm_❤_TensorExpr branch from 22ef336 to e80289b Compare September 29, 2020 15:54
@mruberry mruberry requested review from ZolotukhinM and removed request for apaszke September 29, 2020 15:55
@Krovatkin Krovatkin self-requested a review September 29, 2020 15:57
@t-vi t-vi force-pushed the ROCm_❤_TensorExpr branch from e80289b to aff3290 Compare September 29, 2020 15:57
Copy link
Contributor

@Krovatkin Krovatkin left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@nickgg
Copy link
Contributor

nickgg commented Sep 29, 2020

@t-vi yeah, uh we don't limit the blockDim in the Cuda backend, we just know that (currently) we limit the thread loop to 512 elements. A definite TODO, but for now might make sense to disable it for ROCm and come back to it.

@t-vi
Copy link
Collaborator Author

t-vi commented Sep 29, 2020 via email

Copy link
Contributor

@zheng-xq zheng-xq left a comment

Choose a reason for hiding this comment

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

LGTM in general. Some minor comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please give the constant 128 and 1024 a name here. So people know its meaning.

Also if possible, please include a reference/link where they come from, so future developers know how to update them. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would describing them in the comment more explicitly work or do you prefer a #define or somesuch?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a "static const int kBlcokSizeLimit..." or something like that. I think having both comments and variable names will be even more helpful for future developers. But it is up to you. Thanks!

Copy link
Collaborator Author

@t-vi t-vi Sep 29, 2020

Choose a reason for hiding this comment

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

I think with the new commentary, it should work. It's a much better comment now, thank you for suggesting that it needed improvement.
What do you think?

@t-vi t-vi force-pushed the ROCm_❤_TensorExpr branch 2 times, most recently from a63b39c to f92f089 Compare September 29, 2020 19:34
@t-vi t-vi force-pushed the ROCm_❤_TensorExpr branch from f92f089 to a3dac27 Compare September 29, 2020 19:34
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@Krovatkin merged this pull request in 22a34bc.

@facebook-github-bot
Copy link
Contributor

@Krovatkin merged this pull request in 22a34bc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants