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

Revert "Added tesnorizeation for avx2 based gemm." #4007

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

tqchen
Copy link
Member

@tqchen tqchen commented Sep 25, 2019

@kimishpatel This PR is breaking the CI mainly because of some of the CI machine does not support the cpu feature. Can you send it in again and add additional guard so that it bypasses the test if the CI env does not support the instruction.

@tqchen tqchen merged commit 4a3abb9 into master Sep 25, 2019
@kimishpatel
Copy link
Contributor

kimishpatel commented Sep 25, 2019

@tqchen, sorry about that, but that is strange. It should have skipped this test.

        if not tvm.module.enabled(target):
            print("skip because %s is not enabled..." % target)
            return

I will check what is going on.

@tqchen
Copy link
Member Author

tqchen commented Sep 25, 2019

I think there might be a difference between compiler supporting certain target and cpu has the corresponding feature

@kimishpatel
Copy link
Contributor

I am wondering if CI is running on machine with AVX2 cpu support but the it is disabled. Will have to dig a bit into it.

@tqchen tqchen deleted the revert-3982-tensorization_for_avx2 branch September 25, 2019 23:42
@kimishpatel
Copy link
Contributor

@tqchen, I might be grossly wrong here but I was able to run https://github.com/dmlc/tvm/blob/master/tests/python/contrib/test_gemm_acc16.py on non skylake machine, with the same "illegal instruction" error. Something is weird. This test also checks for the target, so I thought I will look at the corresponding code.
https://github.com/dmlc/tvm/blob/master/src/codegen/llvm/llvm_common.cc#L165
It seems that by the time you get there you never get nullptr? (I am not familiar with llvm codebase but from what I understood it seemed that way). So once you lookup a target and get it, you can create target machine, but lookup does not use cpu string. So it is not clear how to check if particular target is available or not.
Anyways it is unlikely that this is broken but I was trying to understand why the test failed.

@soumith
Copy link

soumith commented Sep 27, 2019

as Tianqi said, compiler supporting the target would be different from guarding if the CPU has that particular feature and skipping out.
Compiler supporting the target would mean compiler can successfully create binary that has AVX2 instructions.
Typically in pytorch, we guard this by runtime-checking if the CPU has AVX2, and if it doesn't, dispatching to the non-AVX routine. For example by using cpuinfo library to check the processor flags: https://github.com/pytorch/pytorch/blob/a23109e12e12db35ca48cf1f991d89304b4bfc73/aten/src/ATen/native/DispatchStub.cpp#L28

@tqchen
Copy link
Member Author

tqchen commented Sep 27, 2019

@kimishpatel if you get a skylake machine and it still gives you illegal instructions, perhaps we need to look into the ASM being generated. You can do it by save the module to a .s file using https://docs.tvm.ai/api/python/module.html?highlight=module%20save#tvm.module.Module.save

@kimishpatel
Copy link
Contributor

@soumith @tqchen that is correct. I was just pointing out that in that case checking if particular feature is supported or not seems broken no? On sky lake machine it runs ok. I think just the way we are checking is broken.

@kimishpatel
Copy link
Contributor

If we are building for the current machine we probably need Cupid of the machine to check if compiled module will be executable.

wweic pushed a commit to wweic/tvm that referenced this pull request Sep 30, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 30, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Oct 1, 2019
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.

3 participants