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

[TARGET] each option of target str should only contain one '=' #5988

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

windclarion
Copy link
Contributor

src/target/target_id.cc ParseAttrsFromRawString L222:
if ((pos = FindUniqueSubstr(s, "=")) != -1)
require option contains only one '='

The CHECK code in FindUniqueSubstr will cause build fail.

Signed-off-by: windclarion windclarion@gmail.com

@junrushao
Copy link
Member

Ah I see, I am a bit surprised the typo is not found before. BTW, please rebase to master, and change “-target” to “-mtriple”. Thanks!

@windclarion
Copy link
Contributor Author

done! old code works ok before "[Target] Migrate data structure of TargetNode (#5960)", the commit add a requirement in src/target/target_id.cc. FindUniqueSubstr require options string has only one '=', we didn't force the limit before that commit.

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

Thanks!

@junrushao
Copy link
Member

@windclarion Looks like a flaky test. Could you file an empty commit using git commit --allow-empty -m "retrigger" to retrigger the CI?

Copy link
Contributor

@ZihengJiang ZihengJiang left a comment

Choose a reason for hiding this comment

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

Could you also add unit tests generating code for those target, which will also re-trigger the CI?

@windclarion
Copy link
Contributor Author

@ZihengJiang I try to add test, but if I feed the option with a string which contains two '=', target_id.cc FindUniqueSubstr CHECK will abort, so I don't know how can I test the abort operation, it's not a exception which I can catch, it's just abort. Can you give me some advice or tell me similar test file which can test the CHECK?

@junrushao
Copy link
Member

@windclarion
Copy link
Contributor Author

@junrushao1994 What I have said maybe not very clear, I mean I can't catch a exception in python code, because C++ CHECK just abort, the abort function didn't throw a exception.

@windclarion
Copy link
Contributor Author

windclarion commented Jul 7, 2020

@junrushao1994 I add a small test which just test target whether can be created, and I just make sure the target shouldn't be None.

@windclarion
Copy link
Contributor Author

windclarion commented Jul 7, 2020

today's test is flaky, I always failed at the unexpected position, and I don't know how can I cause the fail

@junrushao
Copy link
Member

Yeah seems like another flaky test...Could you re-trigger? Thanks!

@junrushao
Copy link
Member

I see. There is a flaky test in the recent week: #6009. Don't worry, let's wait until it is fixed.

@ZihengJiang
Copy link
Contributor

@windclarion Could you try to build a small program likeA+B with every target for testing the code generation? ( you can jump some target like cuda if it requires extra efforts)

@junrushao
Copy link
Member

The flaky test is resolved in #6014 :-)

@windclarion
Copy link
Contributor Author

@ZihengJiang many other tests has tested tvm.build, ex. tests\python\integration\test_reduce.py test_reduce_prims Line64 has checked metal, vulkan, cuda, opencl, rocm

src/target/target_id.cc ParseAttrsFromRawString L222:
if ((pos = FindUniqueSubstr(s, "=")) != -1)
require option contains only one '='

Signed-off-by: windclarion <windclarion@gmail.com>
@ZihengJiang ZihengJiang merged commit 5433b8d into apache:master Jul 9, 2020
@windclarion windclarion deleted the fix_target_check branch July 10, 2020 01:41
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jul 14, 2020
…e#5988)

src/target/target_id.cc ParseAttrsFromRawString L222:
if ((pos = FindUniqueSubstr(s, "=")) != -1)
require option contains only one '='

Signed-off-by: windclarion <windclarion@gmail.com>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jul 14, 2020
…e#5988)

src/target/target_id.cc ParseAttrsFromRawString L222:
if ((pos = FindUniqueSubstr(s, "=")) != -1)
require option contains only one '='

Signed-off-by: windclarion <windclarion@gmail.com>
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.

4 participants