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.

Sign up for GitHub

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

Jump to bottom

[Arith] Simplify cast #7045

Merged
merged 17 commits into from
Jan 7, 2021
Merged

[Arith] Simplify cast #7045

merged 17 commits into from
Jan 7, 2021

Conversation

hzfan
Copy link
Contributor

@hzfan hzfan commented Dec 7, 2020

Follow up of #6691
Simplify cast(i32, c * 2 + 1) + 1 - cast(i32, c * 2) to 2 by first transforming to cast(i32, c * 2) + cast(i32, 1) + 1 - cast(i32, c * 2)

@tqchen
Copy link
Member

tqchen commented Dec 11, 2020

cc @merrymercy @yzhliu @kazum @ZihengJiang can you please help to take a look?

@tqchen tqchen added the status: need update need update based on feedbacks label Dec 26, 2020
@tqchen
Copy link
Member

tqchen commented Dec 26, 2020

cc @hzfan please update per review comments

@hzfan
Copy link
Contributor Author

hzfan commented Dec 28, 2020

@junrushao1994 I just fixed the macro thing and CI's green. Please take another look.

@junrushao
Copy link
Member

It looks good on my side. @yzhliu would you mind taking a look? Thanks!

@junrushao junrushao linked an issue Dec 31, 2020 that may be closed by this pull request
[Performance] Performance regression with int64 indices INDEX_DEFAULT_I64=ON (PR #6143) #6691
Closed
tqchen
tqchen reviewed Dec 31, 2020
View reviewed changes
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

Some more comments to improve readability after carefully read the PR.

src/arith/canonical_simplify.cc Outdated Show resolved Hide resolved
src/arith/canonical_simplify.cc Outdated
@@ -77,6 +77,25 @@ inline PrimExpr DivImpl(PrimExpr a, PrimExpr b, DivMode mode) {
}
}

bool CheckCastImpl(DataType dtype, PrimExpr value, Analyzer* analyzer) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps rename to UpcastIsSafe?

Copy link
Member

Choose a reason for hiding this comment

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

document this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess CastIsSafe seems better, since it checks both upcast and downcast

src/arith/canonical_simplify.cc Outdated Show resolved Hide resolved
src/arith/canonical_simplify.cc Outdated Show resolved Hide resolved
src/arith/canonical_simplify.cc Outdated Show resolved Hide resolved
src/arith/canonical_simplify.cc Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Dec 31, 2020

Thanks @hzfan for keep polishing the code :) Arith analysis is quite core to most of our transformations so it is important to make sure code is clean and readable

I have carefully read the PR and add a few more comments to improve readability.

@junrushao1994 @yzhliu It would be great to also have you take another look as well.

tqchen
tqchen reviewed Jan 5, 2021
View reviewed changes
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

some final comments

src/arith/canonical_simplify.cc Outdated Show resolved Hide resolved
src/arith/canonical_simplify.cc Outdated Show resolved Hide resolved
src/arith/canonical_simplify.cc Outdated Show resolved Hide resolved
@tqchen tqchen merged commit 9815ae2 into apache:main Jan 7, 2021
@tqchen
Copy link
Member

tqchen commented Jan 7, 2021

Thanks @hzfan @junrushao1994 ! This is now merged

@tqchen tqchen added status: accepted and removed status: need review status: need update need update based on feedbacks labels Jan 7, 2021
tkonolige pushed a commit to tkonolige/incubator-tvm that referenced this pull request Jan 11, 2021
@hzfan @tkonolige
[Arith] Simplify cast (apache#7045)
40abc86
masahi pushed a commit to masahi/tvm that referenced this pull request Jan 14, 2021
@hzfan @masahi
[Arith] Simplify cast (apache#7045)
38a29e1
TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Jan 20, 2021
@hzfan
[Arith] Simplify cast (apache#7045)
2bb01fb
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jan 21, 2021
@hzfan @trevor-m
[Arith] Simplify cast (apache#7045)
e8d0897
electriclilies pushed a commit to electriclilies/tvm that referenced this pull request Feb 18, 2021
@hzfan @electriclilies
[Arith] Simplify cast (apache#7045)
d57a309
@junrushao junrushao mentioned this pull request Nov 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junrushao junrushao junrushao left review comments

@tqchen tqchen tqchen approved these changes

Assignees
No one assigned
Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Performance] Performance regression with int64 indices INDEX_DEFAULT_I64=ON (PR #6143)
3 participants
@hzfan @tqchen @junrushao