Skip to content

Comments

[WIP] Just want use official CI to verify idea.#8090

Closed
Johnson9009 wants to merge 2 commits intoapache:mainfrom
Johnson9009:index_merge_mul_mod
Closed

[WIP] Just want use official CI to verify idea.#8090
Johnson9009 wants to merge 2 commits intoapache:mainfrom
Johnson9009:index_merge_mul_mod

Conversation

@Johnson9009
Copy link
Contributor

Just want use official CI to verify idea.

@tqchen
Copy link
Member

tqchen commented May 20, 2021

Please avoid using CI for just verifications purposes, since CI is a resource shared by the community for code contribution gatting, since we always recommend testing things locally before sending the PR and write unit-test cases for the related change.

I can see the proposed set of changes, the particular logic of mul mod merging can likely be removed in most settings where the offset are constant(as in your case), because canonical simplifier covers that.

It is a bit unclear whether we can safely remove in settings when shape are symbolic. Constructing unit-test cases would be a good way to explore the problem

Thank you!

@Johnson9009
Copy link
Contributor Author

Sorry for occupation of CI resources, this idea actually come from a real bug of "ElemOffset", the current mul mod merging logic and simplifier prefer to simplify the most inner expression first, so #8093 will happen. After I know how to fix it, I strange whether we still need the code of mul mod merging, because as you said know the simplifiers of analyzer is very strong, so I have a doubt that we need the code of mul mod merging because we haven't the strong simplifier at that time.
Finally I simple delete the code of mul mod merging and want use official CI to see which case will fail, sorry for that because our internal CI haven't be set up.

@tqchen
Copy link
Member

tqchen commented May 21, 2021

I see, please feel free to send a PR that delete this logic and see if simplifer can handle the case. Thanks @Johnson9009

@Johnson9009 Johnson9009 deleted the index_merge_mul_mod branch August 3, 2021 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants