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

CUDA BFloat16 signal windows #45155

Closed
wants to merge 9 commits into from
Closed

CUDA BFloat16 signal windows #45155

wants to merge 9 commits into from

Conversation

zasdfgbnm
Copy link
Collaborator

Looks like this op is never tested for the support of different dtypes?

@ailzhang ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 22, 2020
@dr-ci
Copy link

dr-ci bot commented Oct 14, 2020

💊 CI failures summary and remediations

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


  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See CircleCI build pytorch_linux_backward_compatibility_check_test (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

Oct 22 00:20:08 The PR is introducing backward incompatible changes to the operator library. Please contact PyTorch team to confirm whether this change is wanted or not.
Oct 22 00:20:08 processing existing schema:  __setstate__(__torch__.torch.classes.quantized.EmbeddingPackedParamsBase _0, (int, Tensor[], float[], int[]) _1) -> (None _0) 
Oct 22 00:20:08 processing existing schema:  bit_rate(__torch__.torch.classes.quantized.EmbeddingPackedParamsBase _0) -> (int _0) 
Oct 22 00:20:08 processing existing schema:  version(__torch__.torch.classes.quantized.EmbeddingPackedParamsBase _0) -> (int _0) 
Oct 22 00:20:08 processing existing schema:  __getstate__(__torch__.torch.classes.xnnpack.LinearOpContext _0) -> ((Tensor, Tensor?, Scalar?, Scalar?) _0) 
Oct 22 00:20:08 processing existing schema:  __setstate__(__torch__.torch.classes.xnnpack.LinearOpContext _0, (Tensor, Tensor?, Scalar?, Scalar?) _1) -> (None _0) 
Oct 22 00:20:08 processing existing schema:  __getstate__(__torch__.torch.classes.xnnpack.Conv2dOpContext _0) -> ((Tensor, Tensor?, int[], int[], int[], int, Scalar?, Scalar?) _0) 
Oct 22 00:20:08 processing existing schema:  __setstate__(__torch__.torch.classes.xnnpack.Conv2dOpContext _0, (Tensor, Tensor?, int[], int[], int[], int, Scalar?, Scalar?) _1) -> (None _0) 
Oct 22 00:20:08 processing existing schema:  __getstate__(__torch__.torch.classes.xnnpack.TransposeConv2dOpContext _0) -> ((Tensor, Tensor?, int[], int[], int[], int[], int, Scalar?, Scalar?) _0) 
Oct 22 00:20:08 processing existing schema:  __setstate__(__torch__.torch.classes.xnnpack.TransposeConv2dOpContext _0, (Tensor, Tensor?, int[], int[], int[], int[], int, Scalar?, Scalar?) _1) -> (None _0) 
Oct 22 00:20:08 processing existing schema:  __init__(__torch__.torch.classes.dist_rpc.WorkerInfo _0, str _1, int _2) -> (None _0) 
Oct 22 00:20:08 The PR is introducing backward incompatible changes to the operator library. Please contact PyTorch team to confirm whether this change is wanted or not.  
Oct 22 00:20:08  
Oct 22 00:20:08 Broken ops: [ 
Oct 22 00:20:08 	prim::is_vulkan(Tensor a) -> (bool) 
Oct 22 00:20:08 ] 
Oct 22 00:20:08 + cleanup 
Oct 22 00:20:08 + retcode=1 
Oct 22 00:20:08 + set +x 
Oct 22 00:20:08 =================== sccache compilation log =================== 
Oct 22 00:20:08 =========== If your build fails, please take a look at the log above for possible reasons =========== 
Oct 22 00:20:08 Compile requests                 0 

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 16 times.

@ngimel
Copy link
Collaborator

ngimel commented Oct 15, 2020

it is tested in test_signal_window_functions, please update the test accordingly.

@zasdfgbnm
Copy link
Collaborator Author

test added for all dtypes, and arange is required for hann. arange will be tested in #44848

@zasdfgbnm zasdfgbnm changed the title CUDA BFloat16 kaiser_window CUDA BFloat16 signal windows Oct 20, 2020
});
const scalar_t alpha = static_cast<scalar_t>((window_length - 1) / 2.0);
gpu_kernel(iter, [=]GPU_LAMBDA(scalar_t a) -> scalar_t {
return calc_i0(static_cast<scalar_t>(beta) * ::sqrt(1 - ::pow((a - alpha) / alpha, static_cast<scalar_t>(2.0)))) / calc_i0(static_cast<scalar_t>(beta));
Copy link
Collaborator

Choose a reason for hiding this comment

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

the intermediate computations of i0 args here should still be in accscalar_t? I can merge as though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is i0 bw bound or compute bound? If it is not bw bound, does it still make sense to compute on accscalar_t?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know, but it's doing internal computations in fp32 anyway, and here

static_cast<scalar_t>(beta) * ::sqrt(1 - ::pow((a - alpha) / alpha

is still doing repeated conversions and truncations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

inline c10::BFloat16 calc_i0(c10::BFloat16 a) { return calc_i0(static_cast<float>(a)); }

OK, calc_i0 is already computing on float32 anyway

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.

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

@zasdfgbnm
Copy link
Collaborator Author

ping @ngimel

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.

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

@zasdfgbnm zasdfgbnm deleted the bfloat-kaiser_window branch October 26, 2020 23:06
@facebook-github-bot
Copy link
Contributor

@ngimel merged this pull request in 99cf3b1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants