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

[BACKPORT-0.6][Bugfix][Arith] keep div_mode during floordiv simplify #5922

Merged
merged 1 commit into from Jun 26, 2020

Conversation

yzhliu
Copy link
Member

@yzhliu yzhliu commented Jun 25, 2020

@tqchen @sergei-grechanik please take a look.

@tqchen
Copy link
Member

tqchen commented Jun 25, 2020

Thanks @yzhliu , seems we need a different testcase, that uses a truncdiv but still triggers DivModeCompatibleTo

@tqchen tqchen added the status: need test case need test cases to cover the change label Jun 25, 2020
@ANSHUMAN87
Copy link
Contributor

ANSHUMAN87 commented Jun 25, 2020

@yzhliu : The original issue is fixed indeed, Thanks!
I think it would be good if we add the issue test case as well.

--> expr = (((x0 - floordiv((0 - (x15)), 2)) - 1) + floordiv((37 + ((x1 + 7)-5)), 2))

@ANSHUMAN87
Copy link
Contributor

ANSHUMAN87 commented Jun 25, 2020

@tqchen, @yzhliu : I have one concern here(However not related to this PR).
As you see the expr below:
expr = (((x0 - floordiv((0 - (x15)), 2)) - 1) + floordiv((37 + ((x1 + 7)-5)), 2))

should be evaluated to actually
expr = x0

but with current simplify stage(rewrite_simplify + canonical_simplify), it evaluates to below:
expr = ((floordiv((0 - (x1: int325)), 2) + x0: int32) - floordiv((x1-5), 2))

But when i run stages as (rewrite_simplify + canonical_simplify + rewrite_simplify) it evaluates perfectly to x0.

So may be we need to run these stages with some customized config every time or until there are no more changes?

@yzhliu
Copy link
Member Author

yzhliu commented Jun 25, 2020

@ANSHUMAN87 good catch, this is what the argument step for I added in #5618

@yzhliu yzhliu force-pushed the fix_floordiv_canonical_simplify branch from 692be9c to c8b43c8 Compare June 25, 2020 17:43
@tqchen
Copy link
Member

tqchen commented Jun 25, 2020

@yzhliu please also send a patch to v0.6

@yzhliu yzhliu force-pushed the fix_floordiv_canonical_simplify branch from 6240c55 to d818f05 Compare June 25, 2020 18:59
@yzhliu yzhliu merged commit 16c5d4d into apache:master Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need test case need test cases to cover the change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants