[te] Fix casting of unsigned char, and abs(int) #44157
Conversation
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 0dd0f65 (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis 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. This comment has been revised 26 times. |
facebook-github-bot
added
the
oncall: jit
[ghstack-poisoned]
ZolotukhinM
left a comment
ZolotukhinM
left a comment
There was a problem hiding this comment.
Looks good! I guess you'll need to replace self.assertEqual(ref, t(x)) with np.testing.assert_allclose(...) to appease the CI gods though.
| torch.int64, | ||
| torch.float16, | ||
| torch.bfloat16, | ||
| # torch.float16, |
There was a problem hiding this comment.
Nit: maybe add a comment whether we want these to be supported at all?
There was a problem hiding this comment.
Oh, yeah, float16 should be coming in soon-ish. The commented lines are a transient state for this stack :-)
|
|
||
| void CudaPrinter::visit(const Cast* v) { | ||
| os() << cudaDtypeCppString(v->dtype()); | ||
| os() << "(" << cudaDtypeCppString(v->dtype()) << ")"; |
There was a problem hiding this comment.
I don't know when it became a thing but apparently you can cast by saying float(x), like it's a constructor. But that sorta doesn't parse if you do unsigned char(x). Clearly we need whitespace in identifiers! (I hope this reddit post is a joke: https://www.reddit.com/r/ProgrammerHumor/comments/4on81i/til_c_allows_u200b_zero_width_space_in_identifiers/ )
[ghstack-poisoned]
Differential Revision: [D23528507](https://our.internmc.facebook.com/intern/diff/D23528507) [ghstack-poisoned]
| if (returnType == ScalarType::Half || returnType == ScalarType::Float) { | ||
| func_name = func_name + "f"; | ||
| } | ||
| if (v->op_type() == IntrinsicsOp::kFabs && is_integral(returnType)) { |
There was a problem hiding this comment.
does nvrtc not handle std::abs or ::abs? All this exp/expf is so last decade (and is not used in eager part of the codebase at all).
There was a problem hiding this comment.
I think it does, actually. I don't know why we went the + "f" route, except that maybe the old fuser uses it? I'll take a note to use std:: in a follow-up.
Differential Revision: [D23528507](https://our.internmc.facebook.com/intern/diff/D23528507) [ghstack-poisoned]
Codecov Report
@@ Coverage Diff @@
## gh/bertmaher/15/base #44157 +/- ##
=======================================================
Coverage ? 69.29%
=======================================================
Files ? 381
Lines ? 47652
Branches ? 0
=======================================================
Hits ? 33022
Misses ? 14630
Partials ? 0 Continue to review full report at Codecov.
|
Differential Revision: [D23528507](https://our.internmc.facebook.com/intern/diff/D23528507) [ghstack-poisoned]
Differential Revision: [D23528507](https://our.internmc.facebook.com/intern/diff/D23528507) [ghstack-poisoned]
|
@bertmaher merged this pull request in 960c088. |
Stack from ghstack:
Differential Revision: D23528507